diff --git a/haproxy_manager.py b/haproxy_manager.py index 2780673..b8b6a0b 100644 --- a/haproxy_manager.py +++ b/haproxy_manager.py @@ -373,8 +373,9 @@ def cert_backup_dir(): # ENCRYPTED PRIVATE KEY is deliberately absent: HAProxy cannot use a # passphrase-protected key from a crt file, so a bundle containing one is not -# publishable. Accepting it here would let one through on a host without the -# openssl CLI, where the pairing check (which would also reject it) is skipped. +# publishable, and the structural layer is the only layer that names the +# problem ("no complete private key block") rather than reporting it as an +# unreadable key. _PEM_KEY_LABELS = ('PRIVATE KEY', 'RSA PRIVATE KEY', 'EC PRIVATE KEY') @@ -406,6 +407,11 @@ def validate_pem_structure(text): lack of a tool. It catches every failure mode the truncation bug produced: empty file, certificate without a key, key without a certificate, and a block cut off mid-write. + + It is NOT sufficient on its own, which is why _openssl_pairing_status() is + mandatory rather than best-effort: a bundle of EMPTY pem blocks (a BEGIN + line immediately followed by its END line, no base64 between them) passes + every check here and is rejected only by openssl. """ if not text.strip(): return False, 'bundle is empty' @@ -430,14 +436,34 @@ def _openssl_pairing_status(path): assembled bundle directly. Comparing the two public keys proves the pair. 'unavailable' means the openssl BINARY is absent - a verdict about our - tooling, not about the bundle. The Dockerfile installs haproxy, certbot, - socat and curl but not the openssl CLI, so this is a real possibility. - Callers treat it as a loud warning rather than a failure: the failure modes - this whole module exists to prevent (truncation, missing key, partial - write) are fully covered by validate_pem_structure(), whereas refusing to - publish whenever the checker is missing would stall renewals fleet-wide and - let certificates expire - a guaranteed outage traded for a hypothetical - one. A mismatch, when we CAN check, is always fatal. + tooling, not about the bundle. validate_pem_bundle() treats it as a HARD + FAILURE. + + That is a deliberate reversal. This docstring used to say "the Dockerfile + installs haproxy, certbot, socat and curl but not the openssl CLI, so this + is a real possibility", and callers accepted the bundle on the structural + checks alone. The premise is false: openssl 3.x IS in the image, as a + dependency of ca-certificates (which certbot requires), and + generate_self_signed_cert() below already runs `openssl req` with + check=True during first-run setup - so no container has ever reached a + publish without it. The 'unavailable' branch never fired, which means the + pairing check has in fact always run, and THAT is what made the fail-open + harmless - not the stated reasoning. Structural validation on its own is + weak: a bundle of empty pem blocks passes validate_pem_structure() and is + caught only here. + + So an absent openssl now means the image is broken, and we say so and stop + instead of quietly downgrading to the weaker check. The cost is that a + hypothetical openssl-less image stops publishing renewals - but it does so + immediately and loudly, in the monitored error log, at the first renewal, + rather than 90 days later; and publishing an unverified bundle can take the + whole :443 bind, i.e. every site on the host, down at the next reload. + + There is no python `cryptography` fallback on purpose: this process 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, and reaching for that interpreter would be a second unverified + premise of exactly the kind this comment is correcting. """ try: cert_pub = subprocess.run( @@ -471,14 +497,17 @@ def _openssl_pairing_status(path): def validate_pem_bundle(path): """Validate a bundle file on disk. Returns (ok, message). - Mandatory structural validation plus a best-effort cryptographic pairing - check - see _openssl_pairing_status() for what happens when openssl is - missing (loud warning, structural verdict stands). + Structural validation AND the cryptographic pairing check, both mandatory - + see _openssl_pairing_status() for why a missing openssl is a failure rather + than a downgrade to structure-only. """ try: with open(path, 'r') as fh: text = fh.read() - except OSError as e: + except (OSError, UnicodeDecodeError) as e: + # UnicodeDecodeError, not just OSError: a bundle corrupted into binary + # is unreadable as text but reads perfectly well as bytes, so `except + # OSError` let the decode error escape as an unhandled traceback. return False, f'cannot read assembled bundle: {e}' ok, msg = validate_pem_structure(text) @@ -489,9 +518,13 @@ def validate_pem_bundle(path): if status == 'invalid': return False, pair_msg if status == 'unavailable': - logger.warning( - "Certificate key/leaf pairing check SKIPPED for %s (%s). The " - "bundle passed structural validation only.", path, pair_msg) + logger.error( + "Certificate key/leaf pairing check could not run for %s (%s) - " + "REFUSING to publish. openssl is required; structural validation " + "alone cannot tell a real bundle from empty pem blocks.", + path, pair_msg) + return False, (f'cert/key pairing check unavailable ({pair_msg}); ' + 'refusing to publish on structural checks alone') return True, None @@ -511,7 +544,14 @@ def backup_existing_pem(dest_path): try: with open(dest_path, 'r') as fh: ok, msg = validate_pem_structure(fh.read()) - except OSError as e: + except (OSError, UnicodeDecodeError) as e: + # UnicodeDecodeError, not just OSError. A live pem corrupted into + # BINARY (the exact state a republish is meant to heal) raises + # UnicodeDecodeError here, and with only OSError caught it escaped all + # the way out of publish_pem_bundle() - so the one operation that would + # have put a working certificate back blew up on the way to taking a + # backup of the broken one. The shell half recovers from this fine; + # this is the only reason the Python half did not. ok, msg = False, str(e) backup_path = os.path.join(cert_backup_dir(), os.path.basename(dest_path)) if not ok: @@ -552,7 +592,9 @@ def publish_pem_bundle(dest_path, source_paths): try: with open(src, 'r') as fh: data = fh.read() - except OSError as e: + except (OSError, UnicodeDecodeError) as e: + # Binary-corrupt source material must be a clean, reported refusal, + # not an unhandled UnicodeDecodeError out of the request handler. raise CertificatePublishError(f'cannot read {src}: {e}') if not data.strip(): raise CertificatePublishError(f'source file is empty: {src}') diff --git a/scripts/cert-publish-lib.sh b/scripts/cert-publish-lib.sh index 58a382d..a589f05 100644 --- a/scripts/cert-publish-lib.sh +++ b/scripts/cert-publish-lib.sh @@ -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 /dev/null /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 `..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" \ diff --git a/scripts/renew-certificates.sh b/scripts/renew-certificates.sh index e646275..f2a421c 100644 --- a/scripts/renew-certificates.sh +++ b/scripts/renew-certificates.sh @@ -108,5 +108,18 @@ if [ $UPDATED -gt 0 ]; then fi fi +# A per-domain publication failure is a real failure and must be reported as +# one. This script used to `exit 0` no matter how many domains failed, so +# "0 updated, 12 failed" - a host that has completely stopped publishing +# renewals - looked identical to a clean run to cron, to +# host-renew-certificates.sh (which branches on this exit code) and to any +# external monitoring. The first visible symptom would have been certificates +# expiring. The loop above deliberately continues past a failed domain so the +# others still get published; the status is reported here instead. +if [ "$FAILED" -gt 0 ]; then + log_error "Certificate renewal process completed with failures: $UPDATED updated, $FAILED failed" + exit 1 +fi + log_info "Certificate renewal process completed" exit 0 diff --git a/scripts/sync-certificates.sh b/scripts/sync-certificates.sh index e84dd1f..80cc038 100755 --- a/scripts/sync-certificates.sh +++ b/scripts/sync-certificates.sh @@ -90,5 +90,12 @@ if [ $UPDATED -gt 0 ]; then fi fi +# See the matching block in renew-certificates.sh: a run in which every domain +# failed to publish must not look like a clean run to its caller. +if [ "$FAILED" -gt 0 ]; then + log_error "Certificate sync process completed with failures: $UPDATED updated, $FAILED failed" + exit 1 +fi + log_info "Certificate sync process completed" exit 0 diff --git a/scripts/test-cert-scripts.py b/scripts/test-cert-scripts.py index 3133280..bc6a240 100755 --- a/scripts/test-cert-scripts.py +++ b/scripts/test-cert-scripts.py @@ -41,16 +41,26 @@ binaries on PATH. The certificate material below is a real self-signed test certificate with its matching key (plus a second, unrelated key for the mismatch case), embedded as -constants so the tests need no openssl to *create* material. The one test that -needs openssl to *verify* pairing skips itself if the binary is absent. +constants so the tests need no openssl to *create* material. openssl IS needed +to run them: the library's cert/key pairing check is mandatory (it is the only +layer that can reject a bundle of empty pem blocks), so without the binary +every publish is refused by design. The image ships openssl 3.x. + +The acceptance bar for this file is a green run INSIDE the built image, as +root, which is where these scripts actually execute - not on a workstation. +Several failure modes are invisible outside the container (root ignores the +directory permissions one test used to rely on) and one was actively +destructive there; see _cleanup_tmp(). """ import os import re import shutil +import stat import subprocess import tempfile import textwrap +import time import unittest MODULE_DIR = os.path.abspath( @@ -152,11 +162,72 @@ FV0RvuQPDYkqsx8ibqpSv7c= -----END PRIVATE KEY----- """ +# A SECOND, unrelated but internally consistent pair. Needed by the +# concurrency test: two bundles that are each perfectly valid but whose keys +# differ, so a validator that reads the certificate from one and the key from +# the other reports a mismatch. Two bundles sharing key material - which is +# what PREVIOUS_BUNDLE and NEW_BUNDLE are - cannot detect that at all. +# openssl req -x509 -newkey rsa:2048 -nodes -days 3650 \ +# -subj /CN=second.example.com +TEST_CERT_2 = """\ +-----BEGIN CERTIFICATE----- +MIIDGzCCAgOgAwIBAgIUKQAAvrVkden7gIg2zYMLb6dO1c8wDQYJKoZIhvcNAQEL +BQAwHTEbMBkGA1UEAwwSc2Vjb25kLmV4YW1wbGUuY29tMB4XDTI2MDgwNjE2NTcz +NVoXDTM2MDgwMzE2NTczNVowHTEbMBkGA1UEAwwSc2Vjb25kLmV4YW1wbGUuY29t +MIIBIjANBgkqhkiG9w0BAQEFAAOCAQ8AMIIBCgKCAQEAv2pfTOvN2zrZJ6d+biiD +VOczMqanuHPBCy2CnTBif+7VWf8AaywRzQ3ShfhmalVRNEFMn0MSO+GocmH71Ve1 +nl89oGiJPmvX1lpncbgM692ddhP9ez4xUeNj+QAWp9VBZhInNuM4Pawv5BPngtpj +2MGXf3ZlBSli8Ng7jBo1fTMT3bh8GcE1rIPRvmUuQwFIt2eGnLR8jQd+xGelhAjG +nnXtlc+ebo4r2OjljNgvtdUknBZdpiZXmjdFzyClYTeMuEen2uwMpJNc0wLbRjcU +khVF3nw4jUnkOhWH3JYGAoWslJyEqZSANwt/eOHwXgyVuxg31bCl297iskW0IRZL +wQIDAQABo1MwUTAdBgNVHQ4EFgQU7HH8GacJzc2j9s2UJCPwykgrIckwHwYDVR0j +BBgwFoAU7HH8GacJzc2j9s2UJCPwykgrIckwDwYDVR0TAQH/BAUwAwEB/zANBgkq +hkiG9w0BAQsFAAOCAQEAYE5jHX1dK091jVsFSZDdiw9AU5rrk8XpF1yuPmDisRnE +dJ4QQq3dzWXRnp0bzZnq7fdfiEz1m39zVixov7WFp24QhenD2n5K7/wew7RpXTnA +pAGBEdsGvBJ+3MgkRYklXCM9f9f4z21xXRNZ+BwBcM25D+gR4b+PRMQR6BhZx5R+ +y2jQsoM68cFjRApFWgmji4pBjg/eOaZMBCfVTjP+npVyqG7UtV5EyYXwPgPa/rm2 +FoP+eftzP6dszBEonkIVyyvkdscI4Wkr8hw3S0R/TP8l9lTnvN1HC3o7Es5VY53R +swNAXWBlgm0N7A96ISLtQjgvOfeMRTCSjxW9pm0wJA== +-----END CERTIFICATE----- +""" + +TEST_KEY_2 = """\ +-----BEGIN PRIVATE KEY----- +MIIEvgIBADANBgkqhkiG9w0BAQEFAASCBKgwggSkAgEAAoIBAQC/al9M683bOtkn +p35uKINU5zMypqe4c8ELLYKdMGJ/7tVZ/wBrLBHNDdKF+GZqVVE0QUyfQxI74ahy +YfvVV7WeXz2gaIk+a9fWWmdxuAzr3Z12E/17PjFR42P5ABan1UFmEic24zg9rC/k +E+eC2mPYwZd/dmUFKWLw2DuMGjV9MxPduHwZwTWsg9G+ZS5DAUi3Z4actHyNB37E +Z6WECMaede2Vz55ujivY6OWM2C+11SScFl2mJleaN0XPIKVhN4y4R6fa7Aykk1zT +AttGNxSSFUXefDiNSeQ6FYfclgYChayUnISplIA3C3944fBeDJW7GDfVsKXb3uKy +RbQhFkvBAgMBAAECggEABQLqy6eedRPt31sLOMFEDkzbZdFHOMpZeuqThsNmjo7z +t9diokgeD4ZQXimbEQqsZYDAtmFGdiEp86I9JQBk/4TUiqFhHrOADBe1jQAptjfs +iyIb51gIPnjZ1RBA5Al8zohy28T9h1+Z7+/OeCvcgLyVAixf/U9pU8D1El9cu9zv +4q4WJPB5Tgkq+YwcmeuT8LzsKoSDmPQjFVY9v+gz6hoVUVyP6gswnlFnKjNcmfZU +0CKP90sCAc5mKZv7RyGG920LDU4u2ggnQoK05GhXK8R3amJmoAF3i+xGeTRvNEKe +wDC7NinTG2WDVo6y/FCvFKs0+qqlKww1u39fq166rQKBgQDtjPjvkDkSYFcWFznn +sfsxN5R1cLpxdutLZhfvtpHIj5NkQbmOk0r3LbDAwU+UNVQ6F8jWQVHUsni9jAY6 +3RNRF4ZabC51aqg/Ssj7d5mE4kj7y6Ch/nnSXIogRxLG4bWo2rtG1M2Dt3Ef08R4 +6gjcp9ZmELyVx7H4yXFJGIaDdQKBgQDOSCHFK/ggZzzy4iC7DOq1lrbR+jyk95rh +F5EGzyAkgJ4uYc9TCPkUDxXjWL17r61/obfbW4znaV9fHEKpz3R54osDXln3oLea +BBWkJI3ANe8iNrGDE4FN9to4DdUMFsWX/WEiRyBPIqDy9DTyadblMLTNZjNPuLmg +ZJOB3tRZnQKBgQCjeglya9Eq2Uv1MuSxk2VnmHU9YOed4BXLHKZKXFz1JgFr1GNL +QAguFK536FDIkO62z9lxwR/8fRnkb7F13uBFRSg7oAlU2qKQc/nePI9UyJkrVxXj +hYn2f6K61c6ROZFXc7e/5gDMrXhXS9gA0iZpG8PLF6eAeB39NTwV7p/bZQKBgQCV +v5eEY58FJu0ABVhtcbsRiA+/70EHIRi2Pz1xC/vxg81RLoArb2AiR7FEEa+8kpQJ +C4VFIPjxJXWuvf1G+Os9cFAqadw1/95JWJ29QywEVSL8W2gSF57O0l0oRCJdXEql +Q7O4BppV2HWu6clmEZ+HUgxu77pgLWHUJi9PIExXoQKBgEq6rYBXmsSQermnPTOn +YFx7c2ns97hsjYIbs497+gPW4/xQWwsN76t60SWqjXV4DHJCpi5Tnjo2fk5/OzK+ +HfshC9CKFDk8T0KGQWwaPwqP/OYbqOA88IlJ7xbPdSuJkjefHCtVVEtao6+HNgzm +lniLtDMpU0MLPgB98ClQ4HDA +-----END PRIVATE KEY----- +""" + # The bundle already on disk when a run starts. Same key material, plus a # trailing marker so "the live file was replaced" and "the old file was # archived" can be told apart byte-for-byte. PREVIOUS_BUNDLE = TEST_CERT + TEST_KEY + '# previous bundle\n' NEW_BUNDLE = TEST_CERT + TEST_KEY +OTHER_BUNDLE = TEST_CERT_2 + TEST_KEY_2 # --- stub binaries ----------------------------------------------------------- STUB_CERTBOT = """\ @@ -216,6 +287,12 @@ class CertScriptFixture(unittest.TestCase): """An isolated fake /etc/haproxy + /etc/letsencrypt plus stub binaries.""" def setUp(self): + # Without the library every "the library rejects X" assertion in this + # file can be satisfied by bash exiting 127, so make its absence a + # failure of every test rather than a silent pass of several. + self.assertTrue(os.path.isfile(LIB), + f'{LIB} is missing - nothing below tests anything') + self.tmp = tempfile.mkdtemp(prefix='haproxy-cert-test-') self.addCleanup(self._cleanup_tmp) @@ -246,11 +323,28 @@ class CertScriptFixture(unittest.TestCase): self.error_log = os.path.join(self.tmp, 'haproxy-manager-errors.log') def _cleanup_tmp(self): - # A test may have chmod 000'd a fixture file. + # A test may have chmod 000'd a fixture file, which would stop rmtree. + # + # SYMLINKS ARE SKIPPED, and that is not a nicety. os.chmod() FOLLOWS + # symlinks, and the openssl-availability tests below build a stripped + # PATH directory out of symlinks to real system binaries (/usr/bin/cat, + # /usr/bin/chmod, ...). Walking those with chmod 0600 as root - which is + # how this container runs - stripped the exec bit from a dozen core + # binaries of the machine running the tests, chmod itself included, so + # it could not even be undone from inside the container: every later + # test failed with "/usr/bin/grep: Permission denied" and certificate + # publishing stayed dead until the container was recreated. It never + # showed up on a workstation because an unprivileged chmod of a + # root-owned file fails EPERM and was swallowed by `except OSError`. + # This file ships in the image (COPY scripts /haproxy/scripts), so + # running it in place is a thing an operator will do. for root, dirs, files in os.walk(self.tmp): for name in files: + path = os.path.join(root, name) + if os.path.islink(path): + continue try: - os.chmod(os.path.join(root, name), 0o600) + os.chmod(path, 0o600) except OSError: pass shutil.rmtree(self.tmp, ignore_errors=True) @@ -310,6 +404,27 @@ class CertScriptFixture(unittest.TestCase): self.assertEqual(sorted(os.listdir(self.staging_dir)), [], 'staging file was not cleaned up') + def assert_rejected(self, result, because): + """The library refused, FOR THE STATED REASON. + + `assertNotEqual(rc, 0)` on its own proves nothing about this library. + Delete cert-publish-lib.sh and `. "$1"` fails, cert_bundle_valid is + never defined, bash exits 127 - and a bare rc!=0 assertion passes. Four + tests in TestCertPublishLibrary were doing exactly that; they were + pinning "some bash pipeline failed", not "the bundle was rejected". + """ + output = result.stdout + result.stderr + self.assertNotIn('command not found', output, + 'the shell could not find the function under test - ' + 'this asserts nothing about the library') + self.assertNotEqual(127, result.returncode, + f'exit 127 means "no such command", not "rejected": {output}') + self.assertNotEqual(0, result.returncode, + f'expected a rejection, got success: {output}') + self.assertIn(because, output, + f'rejected, but not for the expected reason ' + f'({because!r} not in output): {output}') + class CertScriptBehaviour: """Behaviour shared by renew-certificates.sh and sync-certificates.sh. @@ -378,8 +493,17 @@ class CertScriptBehaviour: 'HAProxy was reloaded even though nothing was updated') self.assert_certs_dir_is_clean() self.assert_no_staging_leftovers() - self.assertEqual(result.returncode, 0, - 'a per-domain failure should not change the exit code') + # This used to assert returncode == 0 with the comment "a per-domain + # failure should not change the exit code", codifying the script's + # `exit 0`. That is wrong and it is the dangerous kind of wrong: a host + # where EVERY domain fails to publish looked, to cron and to + # host-renew-certificates.sh (which branches on this exit code), + # exactly like a clean run. Nothing would notice until the certificates + # expired. Continuing past a failed domain so the others still get + # published is right; reporting success afterwards is not. + self.assertNotEqual(result.returncode, 0, + 'a domain that failed to publish must be reported ' + 'in the exit code, not just in the log') def break_source_key(self): """Make reading the source key fail, however this environment allows. @@ -424,8 +548,9 @@ class CertScriptBehaviour: def test_mismatched_key_is_rejected(self): if shutil.which('openssl') is None: - self.skipTest('openssl CLI not available: the cert/key pairing ' - 'check is best-effort and is skipped by design') + self.skipTest('openssl CLI not available: without it every publish ' + 'is refused, so this test could not tell a pairing ' + 'rejection from a missing-checker rejection') self.seed_previous_bundle() before = read(self.live_pem) write(self.src_key, UNRELATED_KEY) @@ -504,10 +629,10 @@ class TestSyncCertificates(CertScriptBehaviour, CertScriptFixture): class TestCertPublishLibrary(CertScriptFixture): """Unit-level checks on cert-publish-lib.sh itself.""" - def call(self, snippet, *args): + def call(self, snippet, *args, **env_overrides): return subprocess.run( ['bash', '-c', '. "$1"; shift; ' + snippet, '_', LIB, *args], - env=self.env(), capture_output=True, text=True) + env=self.env(**env_overrides), capture_output=True, text=True) def test_valid_bundle_accepted(self): path = write(os.path.join(self.tmp, 'ok.pem'), NEW_BUNDLE) @@ -515,14 +640,32 @@ class TestCertPublishLibrary(CertScriptFixture): def test_empty_and_missing_bundles_rejected(self): empty = write(os.path.join(self.tmp, 'empty.pem'), '') - self.assertNotEqual(self.call('cert_bundle_valid "$1"', empty).returncode, 0) + self.assert_rejected(self.call('cert_bundle_valid "$1"', empty), + 'is empty') missing = os.path.join(self.tmp, 'nope.pem') - self.assertNotEqual(self.call('cert_bundle_valid "$1"', missing).returncode, 0) + self.assert_rejected(self.call('cert_bundle_valid "$1"', missing), + 'does not exist') def test_key_without_end_marker_rejected(self): truncated = write(os.path.join(self.tmp, 'cut.pem'), TEST_CERT + '-----BEGIN PRIVATE KEY-----\nMIIEvAIB\n') - self.assertNotEqual(self.call('cert_bundle_valid "$1"', truncated).returncode, 0) + self.assert_rejected(self.call('cert_bundle_valid "$1"', truncated), + 'unterminated private key block') + + def test_empty_pem_blocks_are_rejected(self): + """Why the pairing check is mandatory rather than best-effort. + + Every structural check in the library passes on this file: a complete + CERTIFICATE block and a complete PRIVATE KEY block, both with nothing + between BEGIN and END. Only openssl can tell it is not a certificate. + """ + hollow = write(os.path.join(self.tmp, 'hollow.pem'), + '-----BEGIN CERTIFICATE-----\n' + '-----END CERTIFICATE-----\n' + '-----BEGIN PRIVATE KEY-----\n' + '-----END PRIVATE KEY-----\n') + self.assert_rejected(self.call('cert_bundle_valid "$1"', hollow), + 'openssl could not read the certificate') def test_a_broken_live_pem_does_not_overwrite_a_good_backup(self): """Mirrors create_backup(require_valid=True) in haproxy_manager.py. @@ -544,37 +687,218 @@ class TestCertPublishLibrary(CertScriptFixture): 'a good backup was overwritten with an unusable pem') def test_publish_fails_loudly_when_the_rename_cannot_happen(self): - """No silent fallback to writing straight into the certs dir.""" + """No silent fallback to writing straight into the certs dir. + + The failure is injected with a stub `mv` that refuses, rather than by + chmod 0500 on the certs dir: the container these scripts run in is + root, root ignores directory permissions, so the chmod version skipped + itself exactly where it matters and only ever ran on a workstation. + """ self.seed_previous_bundle() before = read(self.live_pem) - os.chmod(self.certs_dir, 0o500) # no writes: mv will fail - self.addCleanup(os.chmod, self.certs_dir, 0o755) - if os.geteuid() == 0: - self.skipTest('root ignores directory permissions') + write(os.path.join(self.bindir, 'mv'), + "#!/bin/sh\n" + "echo \"mv: cannot move '$2': Permission denied\" >&2\n" + "exit 1\n", 0o755) + self.addCleanup(os.unlink, os.path.join(self.bindir, 'mv')) result = self.call('cert_publish "$1" "$2" "$3"', self.src_cert, self.src_key, self.live_pem) - self.assertNotEqual(result.returncode, 0, - 'a failed rename was reported as success') + self.assert_rejected(result, 'NOT falling back to a direct write') self.assertEqual(read(self.live_pem), before, 'the live pem was damaged by a failed rename') self.assert_no_staging_leftovers() + def test_published_pem_keeps_the_mode_of_the_file_it_replaces(self): + """A write-safety fix must not silently re-permission private keys. + + Both directions matter: mktemp stages at 0600, so without the explicit + chmod every publish would tighten a 0644 bundle; and the mode must not + be copied from a symlink (see the next test). + """ + for mode in (0o644, 0o640, 0o600): + with self.subTest(oct(mode)): + write(self.live_pem, PREVIOUS_BUNDLE, mode) + result = self.call('cert_publish "$1" "$2" "$3"', + self.src_cert, self.src_key, self.live_pem) + self.assertEqual(result.returncode, 0, + result.stdout + result.stderr) + self.assertEqual(read(self.live_pem), NEW_BUNDLE) + self.assertEqual( + stat.S_IMODE(os.stat(self.live_pem).st_mode), mode, + 'publishing changed who can read the private key') + + def test_symlinked_live_pem_does_not_become_world_writable(self): + """`stat -c %a` on a symlink reports 0777 - the LINK's mode, not a + permission. Copying that onto the staged bundle put a world-writable + private key in the directory HAProxy serves from. -L is what makes the + preserved mode the mode of the file an operator actually chose. + """ + target = write(os.path.join(self.tmp, 'real-bundle.pem'), + PREVIOUS_BUNDLE, 0o640) + os.symlink(target, self.live_pem) + + result = self.call('cert_publish "$1" "$2" "$3"', + self.src_cert, self.src_key, self.live_pem) + + self.assertEqual(result.returncode, 0, result.stdout + result.stderr) + mode = stat.S_IMODE(os.stat(self.live_pem).st_mode) + self.assertEqual( + 0, mode & 0o022, + f'published bundle is group/world writable ({oct(mode)}) - the ' + f'symlink mode was copied onto a real private key') + self.assertEqual(0o640, mode) + + def test_publish_refuses_when_staging_is_on_another_filesystem(self): + """The header used to claim a cross-device mv "fails loudly and leaves + the live pem alone". GNU mv does no such thing: across filesystems it + copies, so it truncates and writes the DESTINATION first and only then + discovers it cannot finish (ENOSPC being the realistic case) - the very + truncation this library exists to prevent. Both directories are + env-overridable, so the device numbers have to be checked up front. + """ + staging = os.path.join(self.a_dir_on_another_filesystem(), + 'cert-staging') + self.seed_previous_bundle() + before = read(self.live_pem) + + result = self.call('cert_publish "$1" "$2" "$3"', + self.src_cert, self.src_key, self.live_pem, + CERT_STAGING_DIR=staging) + + self.assert_rejected(result, 'different filesystems') + self.assertEqual(read(self.live_pem), before, + 'the live pem was disturbed by a refused publish') + self.assert_certs_dir_is_clean() + + def test_stale_python_side_staging_temps_are_reaped(self): + """The staging dir has two writers. + + write_config_atomically() on the Python side stages as + `..tmp` (tempfile.mkstemp(prefix=name + '.', + suffix='.tmp')). The reaper matched only mktemp's `*.??????` shape, so + every temp leaked by a SIGKILL on the Python side stayed there forever. + """ + os.makedirs(self.staging_dir, exist_ok=True) + stale_py = write(os.path.join(self.staging_dir, + DOMAIN + '.pem.ab12cd34.tmp'), 'stale\n') + stale_sh = write(os.path.join(self.staging_dir, + DOMAIN + '.pem.AbCdEf'), 'stale\n') + fresh = write(os.path.join(self.staging_dir, + 'recent.pem.zz99yy.tmp'), 'fresh\n') + old = time.time() - 3 * 24 * 3600 + for path in (stale_py, stale_sh): + os.utime(path, (old, old)) + + result = self.call('cert_publish "$1" "$2" "$3"', + self.src_cert, self.src_key, self.live_pem) + self.assertEqual(result.returncode, 0, result.stdout + result.stderr) + + self.assertFalse(os.path.exists(stale_py), + 'a stale Python-side staging temp was never reaped') + self.assertFalse(os.path.exists(stale_sh), + 'a stale shell-side staging temp was never reaped') + self.assertTrue(os.path.exists(fresh), + 'the reaper deleted a temp a concurrent publisher may ' + 'still be writing') + + def test_validation_of_a_live_pem_being_republished_is_not_spurious(self): + """cert_bundle_valid must judge ONE snapshot of the file. + + cert_publish() calls cert_bundle_valid() on the LIVE pem (step (e), to + decide whether it is worth backing up) while another publisher may be + renaming a new bundle over it. The function used to open the file six + times, so `openssl x509` could read the outgoing bundle and `openssl + pkey` the incoming one - and report "private key does not match the + certificate" about two files that were each perfectly fine. That ERROR + goes into the log monitor-errors.sh watches, which makes it a page. + + The two bundles alternated below are each internally valid but carry + DIFFERENT key material. That matters: NEW_BUNDLE and PREVIOUS_BUNDLE + share a cert and a key, so alternating those two could never produce a + mismatch no matter how badly the reads were interleaved - the test + would model a world in which the bug cannot happen and pass forever. + """ + write(self.live_pem, NEW_BUNDLE) + a = write(os.path.join(self.tmp, 'churn-a.pem'), NEW_BUNDLE) + b = write(os.path.join(self.tmp, 'churn-b.pem'), OTHER_BUNDLE) + + # A publisher renaming over the live pem as fast as it can. Staged + # outside the certs dir, then renamed, exactly like cert_publish(). + churn = subprocess.Popen( + ['bash', '-c', + 'end=$((SECONDS+8)); s="$4"; while [ $SECONDS -lt $end ]; do ' + ' cp "$1" "$s"; mv -f "$s" "$2"; ' + ' cp "$3" "$s"; mv -f "$s" "$2"; ' + 'done', '_', a, self.live_pem, b, + os.path.join(self.tmp, 'churn-staged.pem')], + env=self.env(), stdout=subprocess.DEVNULL, stderr=subprocess.DEVNULL) + def _stop(): + churn.kill() + churn.wait() + self.addCleanup(_stop) + + result = subprocess.run( + ['bash', '-c', + '. "$1"; for i in $(seq 1 200); do cert_bundle_valid "$2" || exit 1; done', + '_', LIB, self.live_pem], + env=self.env(), capture_output=True, text=True, timeout=120) + _stop() + + output = result.stdout + result.stderr + self.assertNotIn('does not match the certificate', output, + 'two valid bundles were reported as a mismatched pair ' + 'because the checks read different files') + self.assertEqual(0, result.returncode, + f'a concurrent republish made validation fail: {output}') + + def a_dir_on_another_filesystem(self): + certs_dev = os.stat(self.certs_dir).st_dev + for candidate in ('/dev/shm', '/run', '/var/tmp', '/tmp', '/'): + try: + if (os.path.isdir(candidate) + and os.access(candidate, os.W_OK) + and os.stat(candidate).st_dev != certs_dev): + path = tempfile.mkdtemp(prefix='cert-xdev-', dir=candidate) + self.addCleanup(shutil.rmtree, path, True) + return path + except OSError: + continue + self.skipTest('no writable directory on a second filesystem available') + def test_haproxy_config_ok_follows_the_validator(self): self.assertEqual(self.call('haproxy_config_ok').returncode, 0) write(self.haproxy_cfg, GOOD_HAPROXY_CFG + BROKEN_TOKEN + '\n') self.assertNotEqual(self.call('haproxy_config_ok').returncode, 0) - def test_missing_openssl_warns_but_does_not_block(self): - """Best-effort layer: a missing checker must not stall renewals.""" - fake_path = os.path.join(self.tmp, 'no-openssl-bin') + def _openssl_free_path(self, name): + """A PATH directory with the library's tools but no openssl. + + Symlinks, so this directory must never be walked with a chmod that + follows them - see _cleanup_tmp(). + """ + fake_path = os.path.join(self.tmp, name) os.makedirs(fake_path) for tool in ('cat', 'grep', 'mktemp', 'mv', 'cp', 'rm', 'mkdir', - 'basename', 'dirname', 'find', 'date', 'chmod'): + 'basename', 'dirname', 'find', 'date', 'chmod', 'stat'): real = shutil.which(tool) if real: os.symlink(real, os.path.join(fake_path, tool)) + return fake_path + + def test_missing_openssl_is_a_hard_failure(self): + """The pairing check is MANDATORY: no openssl, no publication. + + This used to assert the opposite - that a missing openssl warns and + publishes anyway - justified by "the image does not necessarily install + the openssl CLI". The image does: openssl 3.x arrives with + ca-certificates, which certbot needs, and generate_self_signed_cert() + already runs `openssl req` with check=True at first-run setup. So the + fail-open never actually fired, and structural checks alone accept a + bundle of empty pem blocks (see test_empty_pem_blocks_are_rejected). + """ + fake_path = self._openssl_free_path('no-openssl-bin') path = write(os.path.join(self.tmp, 'ok.pem'), NEW_BUNDLE) # bash by absolute path: the stripped PATH cannot resolve it. @@ -583,29 +907,27 @@ class TestCertPublishLibrary(CertScriptFixture): '_', LIB, path], env=self.env(PATH=fake_path), capture_output=True, text=True) - self.assertEqual(result.returncode, 0, - 'a missing openssl blocked publication') - self.assertRegex(result.stdout + result.stderr, - r'(?i)warning.*openssl', - 'the skipped pairing check was not announced loudly') + self.assert_rejected(result, 'openssl binary not found') + self.assertIn('REFUSING', result.stdout + result.stderr, + 'a broken image must be reported as a broken image') - def test_missing_openssl_still_rejects_a_structurally_broken_bundle(self): - fake_path = os.path.join(self.tmp, 'no-openssl-bin2') - os.makedirs(fake_path) - for tool in ('cat', 'grep', 'date'): - real = shutil.which(tool) - if real: - os.symlink(real, os.path.join(fake_path, tool)) - path = write(os.path.join(self.tmp, 'nokey.pem'), TEST_CERT) + def test_missing_openssl_stops_a_publish_rather_than_weakening_it(self): + """cert_publish must inherit the refusal, and not touch the live pem.""" + fake_path = self._openssl_free_path('no-openssl-bin2') + self.seed_previous_bundle() + before = read(self.live_pem) - # bash by absolute path: the stripped PATH cannot resolve it. result = subprocess.run( - [shutil.which('bash'), '-c', '. "$1"; cert_bundle_valid "$2"', - '_', LIB, path], + [shutil.which('bash'), '-c', + '. "$1"; cert_publish "$2" "$3" "$4"', + '_', LIB, self.src_cert, self.src_key, self.live_pem], env=self.env(PATH=fake_path), capture_output=True, text=True) - self.assertNotEqual(result.returncode, 0, - 'structural checks stopped being mandatory') + self.assert_rejected(result, 'openssl binary not found') + self.assertEqual(read(self.live_pem), before, + 'the live pem was disturbed by a refused publish') + self.assert_certs_dir_is_clean() + self.assert_no_staging_leftovers() class TestScriptsAreSane(unittest.TestCase): diff --git a/scripts/test-cert-write-safety.py b/scripts/test-cert-write-safety.py index 9125f53..9a94ea0 100755 --- a/scripts/test-cert-write-safety.py +++ b/scripts/test-cert-write-safety.py @@ -50,6 +50,7 @@ import logging import os import shutil import sqlite3 +import stat import subprocess import sys import tempfile @@ -90,8 +91,25 @@ finally: logging.getLogger('haproxy_manager').setLevel(logging.CRITICAL) HAS_PUBLISHER = hasattr(hm, 'publish_pem_bundle') -FIX_ONLY = unittest.skipUnless( - HAS_PUBLISHER, 'requires the certificate publishing fix (publish_pem_bundle)') + +# FIX_ONLY marks tests that can only run against a tree that HAS the fix, so +# that pointing HAPROXY_MANAGER_DIR at a pre-fix checkout (how the bugs were +# reproduced) skips them instead of erroring. +# +# It used to be `skipUnless(HAS_PUBLISHER, ...)` unconditionally, which is +# tautological when testing THIS tree: rename or delete publish_pem_bundle and +# 10 of the 17 tests silently skip themselves while the run still reports OK. +# A guard that disappears when the thing it guards disappears is not a guard. +# So the escape hatch is now tied to the thing it exists for - testing a +# FOREIGN tree - and a missing publisher in the repo checkout is a hard +# failure (see TestPublisherApiIsPresent). +_REPO_ROOT = os.path.realpath( + os.path.join(os.path.dirname(os.path.abspath(__file__)), '..')) +TESTING_FOREIGN_TREE = os.path.realpath(MODULE_DIR) != _REPO_ROOT +FIX_ONLY = unittest.skipIf( + TESTING_FOREIGN_TREE and not HAS_PUBLISHER, + 'HAPROXY_MANAGER_DIR points at a tree without the certificate publishing ' + 'fix (publish_pem_bundle)') NEEDS_OPENSSL = unittest.skipUnless( shutil.which('openssl'), 'needs the openssl CLI') @@ -186,6 +204,14 @@ k49ABlblWHBsoUF63ka1PBrMgA== GOOD_BUNDLE = LEAF_CERT + LEAF_KEY +# What a bundle already on disk looks like when a test starts: the same, valid +# material plus a trailing marker. Without it the "previous" and "new" bundles +# are byte-identical, and a test that meant to prove a publish HAPPENED cannot +# tell that from a publish that did nothing at all. Trailing text after the key +# is ignored by both HAProxy and openssl, so the file stays genuinely usable. +PREVIOUS_MARKER = '# previous bundle\n' +PREVIOUS_BUNDLE = GOOD_BUNDLE + PREVIOUS_MARKER + # Stub haproxy. Beyond the config check the parent suite's stub does, this one # also walks the crt directory the way HAProxy does when a `bind ... ssl crt # ` is used: every file in there must be a loadable cert+key bundle, and @@ -375,9 +401,9 @@ class CertPublishTestCase(unittest.TestCase): self.write(os.path.join(live, 'privkey.pem'), key) return live - def publish_live_bundle(self, domain): + def publish_live_bundle(self, domain, content=PREVIOUS_BUNDLE): """A good bundle already being served for `domain`.""" - return self.write(os.path.join(self.certs, f'{domain}.pem'), GOOD_BUNDLE) + return self.write(os.path.join(self.certs, f'{domain}.pem'), content) def add_domain(self, domain, backend_name, ssl_cert_path=None): with sqlite3.connect(hm.DB_FILE) as conn: @@ -478,19 +504,38 @@ class TestLivePemIsNeverTruncated(CertPublishTestCase): self.assertEqual('error', resp.get_json()['status']) def test_successful_renewal_still_publishes(self): - """The guard must not block the normal path.""" + """The guard must not block the normal path. + + The old version built its "renewed" certificate with a no-op + `.replace('\\n-----END CERTIFICATE-----', '\\n-----END CERTIFICATE-----')`, + so the renewed lineage was byte-identical to what was already being + served. Every assertion still passed if the renewal published nothing + at all - which is the failure this test is supposed to catch. The live + bundle now carries a marker the renewed material does not, so "the file + on disk changed" is an actual assertion. + """ cert_path = self.publish_live_bundle('ok.example.com') + self.assertIn(PREVIOUS_MARKER, self.read(cert_path), + 'fixture precondition: the live bundle is distinguishable') self.add_domain('ok.example.com', 'ok_backend', ssl_cert_path=cert_path) - renewed = LEAF_CERT.replace('\n-----END CERTIFICATE-----', - '\n-----END CERTIFICATE-----') - self.make_lineage('ok.example.com', cert=renewed) + self.make_lineage('ok.example.com') resp = self.client.post('/api/certificates/renew') self.assertEqual(200, resp.status_code, resp.get_data(as_text=True)) - self.assertTrue(structurally_valid(self.read(cert_path))) + published = self.read(cert_path) + self.assertEqual(GOOD_BUNDLE, published, + 'the renewed cert+key was not written to the live pem') + self.assertNotIn(PREVIOUS_MARKER, published, + 'the previous bundle is still on disk: the renewal ' + 'published nothing') + self.assertTrue(structurally_valid(published)) self.assertTrue(self.edge_would_start()) self.assert_only_final_pems_in_certs_dir() + backup = os.path.join(hm.cert_backup_dir(), 'ok.example.com.pem') + self.assertTrue(os.path.exists(backup)) + self.assertEqual(PREVIOUS_BUNDLE, self.read(backup), + 'the archived copy is not the bundle that was replaced') class TestOldCertificateIsNotDestroyedFirst(CertPublishTestCase): @@ -658,11 +703,25 @@ class TestBundleValidation(CertPublishTestCase): @FIX_ONLY def test_staging_and_backups_live_outside_the_crt_directory(self): - """A temp or backup file inside the crt dir would be loaded by HAProxy.""" - for path in (hm.cert_staging_dir(), hm.cert_backup_dir()): + """A temp or backup file inside the crt dir would be loaded by HAProxy. + + `startswith(certs + os.sep)` alone let the worst case through: if + cert_staging_dir() returned the crt directory ITSELF - staging straight + into the directory HAProxy scans, the exact hazard this design exists + to prevent - the assertion passed, because the certs dir is not a + strict subpath of itself. + """ + certs = os.path.realpath(self.certs) + for name, path in (('staging', hm.cert_staging_dir()), + ('backup', hm.cert_backup_dir())): + real = os.path.realpath(path) + self.assertNotEqual( + certs, real, + f'the {name} directory IS the crt directory - HAProxy would ' + f'load every temp/backup file in it') self.assertFalse( - os.path.abspath(path).startswith(os.path.abspath(self.certs) + os.sep), - f'{path} must not be inside {self.certs}') + real.startswith(certs + os.sep), + f'the {name} directory {path} must not be inside {self.certs}') @FIX_ONLY def test_previous_bundle_is_backed_up_on_publish(self): @@ -700,18 +759,160 @@ class TestBundleValidation(CertPublishTestCase): 'file') @FIX_ONLY + @NEEDS_OPENSSL def test_no_temp_file_survives_a_failed_publish(self): + """The failure must happen AFTER something has been staged. + + This used to feed publish_pem_bundle() an empty privkey, which is + rejected while reading the sources - before the staging directory is + even created. The test then asserted `[] == []` and proved nothing + about temp-file cleanup. A structurally perfect bundle whose key + belongs to a different certificate is rejected by the pairing check, + which runs on the fully staged file, so the temp definitely exists at + the moment the publish fails. + """ dest = os.path.join(self.certs, 'leak.example.com.pem') cert = self.write(os.path.join(self.tmp, 'src4', 'fullchain.pem'), LEAF_CERT) - empty = self.write(os.path.join(self.tmp, 'src4', 'privkey.pem'), '') + wrong = self.write(os.path.join(self.tmp, 'src4', 'privkey.pem'), + UNRELATED_KEY) with self.assertRaises(hm.CertificatePublishError): - hm.publish_pem_bundle(dest, [cert, empty]) + hm.publish_pem_bundle(dest, [cert, wrong]) + + staging = hm.cert_staging_dir() + self.assertTrue(os.path.isdir(staging), + 'the publish never got as far as staging, so this ' + 'asserts nothing about cleanup') + self.assertEqual([], os.listdir(staging), + 'staged files must be cleaned up when a publish fails') + self.assertFalse(os.path.exists(dest), + 'a rejected bundle was published anyway') self.assert_only_final_pems_in_certs_dir() - self.assertEqual( - [], os.listdir(hm.cert_staging_dir()) if - os.path.isdir(hm.cert_staging_dir()) else [], - 'staged files must be cleaned up when a publish fails') + + + @FIX_ONLY + @NEEDS_OPENSSL + def test_empty_pem_blocks_are_rejected(self): + """Why the pairing check may not be best-effort. + + Structural validation is weak on its own: BEGIN/END pairs with nothing + between them satisfy every rule in validate_pem_structure(). Only + openssl can say this is not a certificate. + """ + hollow = ('-----BEGIN CERTIFICATE-----\n-----END CERTIFICATE-----\n' + '-----BEGIN PRIVATE KEY-----\n-----END PRIVATE KEY-----\n') + ok, _ = hm.validate_pem_structure(hollow) + self.assertTrue(ok, 'precondition: structure alone accepts this file') + + path = self.write(os.path.join(self.tmp, 'hollow.pem'), hollow) + ok, msg = hm.validate_pem_bundle(path) + self.assertFalse(ok, 'a bundle of empty pem blocks was accepted') + + @FIX_ONLY + def test_publish_refuses_when_the_pairing_check_cannot_run(self): + """No fail-open. 'unavailable' means the image is broken, not the cert. + + The previous behaviour logged a warning and published on structural + checks alone, justified by "the Dockerfile does not install the openssl + CLI". It does - openssl 3.x comes in with ca-certificates, and + generate_self_signed_cert() shells out to `openssl req` with + check=True during setup - so this branch never fired and the fail-open + was safe only by accident. + """ + real = hm._openssl_pairing_status + hm._openssl_pairing_status = lambda path: ('unavailable', + 'openssl binary not found') + self.addCleanup(setattr, hm, '_openssl_pairing_status', real) + + dest = self.publish_live_bundle('nocheck.example.com') + before = self.read(dest) + cert = self.write(os.path.join(self.tmp, 'src5', 'fullchain.pem'), + LEAF_CERT) + key = self.write(os.path.join(self.tmp, 'src5', 'privkey.pem'), LEAF_KEY) + + with self.assertRaises(hm.CertificatePublishError): + hm.publish_pem_bundle(dest, [cert, key]) + + self.assertEqual(before, self.read(dest), + 'the live pem was disturbed by a refused publish') + self.assert_only_final_pems_in_certs_dir() + + @FIX_ONLY + def test_binary_corrupt_live_bundle_can_still_be_republished(self): + """Republishing is how an operator heals a corrupt live pem. + + A pem corrupted into binary (a partial write from before this fix, a + bad restore) is unreadable as text. backup_existing_pem() opens it in + text mode and caught only OSError, so the UnicodeDecodeError escaped + publish_pem_bundle() entirely: the single operation that would have put + a working certificate back blew up trying to archive the broken one. + The shell half recovers from this without complaint. + """ + dest = os.path.join(self.certs, 'binary.example.com.pem') + with open(dest, 'wb') as fh: + fh.write(b'\x00\x01\x02\xfe\xff' * 128) + cert = self.write(os.path.join(self.tmp, 'src6', 'fullchain.pem'), + LEAF_CERT) + key = self.write(os.path.join(self.tmp, 'src6', 'privkey.pem'), LEAF_KEY) + + hm.publish_pem_bundle(dest, [cert, key]) + + self.assertEqual(GOOD_BUNDLE, self.read(dest), + 'a binary-corrupt live pem blocked its own repair') + self.assertTrue(self.edge_would_start()) + + @FIX_ONLY + def test_published_pem_file_mode(self): + """Publishing must not silently re-permission a private key. + + The staged file is created by mkstemp at 0600; without the explicit + mode copy in write_config_atomically() every publish would tighten a + 0644 bundle, and a future change in the other direction would loosen + one. Neither is a decision a write-safety fix gets to make as a side + effect, so pin it. + """ + cert = self.write(os.path.join(self.tmp, 'src7', 'fullchain.pem'), + LEAF_CERT) + key = self.write(os.path.join(self.tmp, 'src7', 'privkey.pem'), LEAF_KEY) + + for mode in (0o644, 0o640, 0o600): + with self.subTest(oct(mode)): + dest = self.publish_live_bundle(f'mode{mode:o}.example.com') + os.chmod(dest, mode) + hm.publish_pem_bundle(dest, [cert, key]) + self.assertEqual(mode, + stat.S_IMODE(os.stat(dest).st_mode), + 'publishing changed who can read the key') + + fresh = os.path.join(self.certs, 'fresh.example.com.pem') + hm.publish_pem_bundle(fresh, [cert, key]) + fresh_mode = stat.S_IMODE(os.stat(fresh).st_mode) + self.assertEqual(0o644, fresh_mode, + 'a newly created bundle should match the 0644 that ' + '`cat > file` produced under the standard umask') + self.assertEqual(0, fresh_mode & 0o022, + 'a private key must never be group/world writable') + + +@unittest.skipIf(TESTING_FOREIGN_TREE, + 'HAPROXY_MANAGER_DIR points at another tree') +class TestPublisherApiIsPresent(unittest.TestCase): + """FIX_ONLY must never be able to hide the fix going missing. + + With `FIX_ONLY = skipUnless(hasattr(hm, 'publish_pem_bundle'))`, renaming + that one function turned 10 of these 17 tests into skips and the run still + printed OK. This class fails loudly instead. + """ + + def test_publisher_api_is_present(self): + for name in ('publish_pem_bundle', 'validate_pem_bundle', + 'validate_pem_structure', 'backup_existing_pem', + 'cert_staging_dir', 'cert_backup_dir', + 'CertificatePublishError', '_openssl_pairing_status'): + self.assertTrue( + hasattr(hm, name), + f'haproxy_manager.{name} is gone - the tests that exercise it ' + f'would otherwise skip themselves and report OK') class TestClusterSecretSelfHeal(CertPublishTestCase):