From 9d161511208cb4e8e9ddfd2cc2eb8322424286a5 Mon Sep 17 00:00:00 2001 From: Josh Knapp Date: Thu, 6 Aug 2026 08:22:02 -0700 Subject: [PATCH 1/4] fix(config): take the rollback backup BEFORE writing the new config generate_config() wrote /etc/haproxy/haproxy.cfg and only then called reload_haproxy_safely(), which called create_backup(). The "backup" was therefore a copy of the config that had just been written, so on a validation failure restore_backup() restored the identical broken bytes: the advertised rollback was a no-op and a fatal haproxy.cfg stayed on disk, where start_haproxy() refuses to launch. Same shape as the June 2026 incident where a missing template produced a fatal config and took an edge down. Reproduced end to end before the fix (invalid config generated -> "Backups created successfully" -> "Backups restored successfully" -> haproxy.cfg on disk still invalid, `haproxy -c` rc=1). Changes: * create_backup() is now called by generate_config() BEFORE the first write, which also covers blocked_ips.map (rewritten early in generate_config) and coraza-spoe.cfg - both previously written before the backup and, for the SPOE file, never backed up at all even though `haproxy -c` parses it. * create_backup() refuses to promote a config HAProxy already rejects, so a broken file on disk cannot overwrite a known-good backup ("rollback" must not mean "restore a different broken config"). It returns (ok, status) so the caller knows whether a rollback target exists. * promote_current_config_to_backup() records the config as known-good only after it has validated AND loaded, so a box whose first generation succeeded has a rollback target immediately, and a config that never loaded is never promoted. * restore_backup() returns (restored, message) and distinguishes "no backup available" from "restored". Every caller now surfaces the difference; a failed rollback is logged CRITICAL and reported as ROLLBACK FAILED in the API error message instead of silently looking like a successful recovery. * reload_haproxy_safely(backup_status=...) no longer takes its own backup - it runs after the write, where a backup is meaningless. Called without a status it logs the contract violation rather than overwriting a good backup. * validate_config_file() separates "config is invalid" from "validator could not run" so a missing haproxy binary is not read as a bad config. * Config writes are atomic (temp file + fsync + os.replace, mode preserved); a truncated haproxy.cfg is as fatal as an invalid one. Removes the dead temp_config_path variable whose comment claimed this already happened. * Fast path: if the live config set is already byte-identical to the backup (the normal case after a successful reload), skip the re-validation and the copy, so this adds no `haproxy -c` latency to customer-facing API calls. Tests: scripts/test-config-rollback.py - 17 self-contained stdlib-unittest tests, no new dependencies (the repo has no Python test framework; the existing scripts/test-*.sh are curl integration scripts). A stub `haproxy` binary stands in for the validator. 12 of the 17 fail against the previous code; every assertion was mutation-proven (9 mutations, each reddening only the tests that cover it). No template, QUIC or HTTP/3 changes. --- VERSION | 2 +- haproxy_manager.py | 393 +++++++++++++++++++++++---- scripts/test-config-rollback.py | 467 ++++++++++++++++++++++++++++++++ 3 files changed, 801 insertions(+), 61 deletions(-) create mode 100755 scripts/test-config-rollback.py diff --git a/VERSION b/VERSION index 0657c95..8ac114e 100644 --- a/VERSION +++ b/VERSION @@ -1 +1 @@ -2026.07.3 +2026.08.1 diff --git a/haproxy_manager.py b/haproxy_manager.py index a70f812..b7ef7ef 100644 --- a/haproxy_manager.py +++ b/haproxy_manager.py @@ -12,6 +12,7 @@ from datetime import datetime, timedelta import json import ipaddress import shutil +import stat import tempfile import threading import time @@ -105,6 +106,12 @@ HAPROXY_CONFIG_PATH = '/etc/haproxy/haproxy.cfg' HAPROXY_BACKUP_PATH = '/etc/haproxy/haproxy.cfg.backup' BLOCKED_IPS_MAP_PATH = '/etc/haproxy/blocked_ips.map' BLOCKED_IPS_MAP_BACKUP_PATH = '/etc/haproxy/blocked_ips.map.backup' +# Coraza SPOE engine file. `haproxy -c` parses this too (the frontend's +# `filter spoe engine coraza config ` line points at it), so it is part +# of the same restorable config set as haproxy.cfg — rolling back haproxy.cfg +# while leaving a broken coraza-spoe.cfg behind still fails validation. +CORAZA_SPOE_CONFIG_PATH = '/etc/haproxy/coraza-spoe.cfg' +CORAZA_SPOE_BACKUP_PATH = '/etc/haproxy/coraza-spoe.cfg.backup' HAPROXY_SOCKET_PATH = '/var/run/haproxy.sock' SSL_CERTS_DIR = '/etc/haproxy/certs' # Stable per-host secret for QUIC Retry/address-validation tokens. Lives in the @@ -1795,6 +1802,21 @@ def generate_config(): config_parts = [] + # Snapshot the last-known-good config BEFORE anything below touches a + # file in /etc/haproxy. Everything this function writes (haproxy.cfg, + # blocked_ips.map, coraza-spoe.cfg) is validated as one set by + # `haproxy -c`, so the rollback point has to predate the first of them. + # Taking it here (rather than inside reload_haproxy_safely(), which runs + # after the writes) is what makes rollback real - see create_backup(). + backup_ok, backup_status = create_backup() + if not backup_ok: + # Could not even attempt a snapshot (I/O error). Writing a new + # config now would leave us with no way back, so refuse. + raise Exception( + "Refusing to regenerate config: failed to back up the current " + "configuration, so a failed change could not be rolled back" + ) + # Optional Coraza WAF integration. When HAPROXY_CORAZA_SPOE_BACKEND is # set on the haproxy-manager container, we render an extra TCP backend # pointing at a coraza-spoa sidecar AND inject a `filter spoe ...` line @@ -1984,25 +2006,21 @@ backend default-backend # how the file was authored. if not coraza_spoe_cfg.endswith('\n'): coraza_spoe_cfg += '\n' - coraza_spoe_path = '/etc/haproxy/coraza-spoe.cfg' - with open(coraza_spoe_path, 'w') as f: - f.write(coraza_spoe_cfg) - logger.info(f"Coraza SPOE engine config written to {coraza_spoe_path} " + write_config_atomically(CORAZA_SPOE_CONFIG_PATH, coraza_spoe_cfg) + logger.info(f"Coraza SPOE engine config written to " + f"{CORAZA_SPOE_CONFIG_PATH} " f"(SPOA target: {coraza_spoe_backend})") - # Write complete configuration to tmp - temp_config_path = "/etc/haproxy/haproxy.cfg" - config_content = '\n'.join(config_parts) logger.debug("Generated HAProxy configuration") - # Write complete configuration to tmp - # Write new configuration to file - with open(HAPROXY_CONFIG_PATH, 'w') as f: - f.write(config_content) - + # Write new configuration to file (atomically - a truncated haproxy.cfg + # is as fatal as an invalid one). The rollback point was taken above, + # before this write. + write_config_atomically(HAPROXY_CONFIG_PATH, config_content) + # Use safe reload with validation and rollback - success, message = reload_haproxy_safely() + success, message = reload_haproxy_safely(backup_status=backup_status) if success: logger.info("Configuration generated and HAProxy reloaded safely") log_operation('generate_config', True, 'Configuration generated and HAProxy reloaded safely') @@ -2019,63 +2037,306 @@ backend default-backend traceback.print_exc() raise -def create_backup(): - """Create backup of current config and map files""" +# --------------------------------------------------------------------------- +# Config backup / rollback +# --------------------------------------------------------------------------- +# Rollback only works if the backup predates the write it is supposed to undo. +# Until 2026-08 create_backup() ran from inside reload_haproxy_safely(), i.e. +# AFTER generate_config() had already overwritten haproxy.cfg — so the "backup" +# was a copy of the new (possibly broken) config and restore_backup() restored +# the same broken bytes. The advertised rollback was a no-op and a fatal +# haproxy.cfg persisted on disk, where start_haproxy() refuses to launch (the +# June 2026 missing-template incident). create_backup() must now be called by +# the writer, BEFORE the first byte is written. + +# Statuses returned by create_backup() that mean a rollback target exists. +_ROLLBACK_AVAILABLE_STATUSES = ('created', 'kept_previous') + + +def _files_identical(path_a, path_b): + """Byte-compare two files. + + Deliberately not filecmp.cmp(): it memoises on (size, mtime), and + shutil.copy2() preserves mtime, so a stale cache entry could report a + changed config as unchanged. These files are small; read them. + """ try: - if os.path.exists(HAPROXY_CONFIG_PATH): - shutil.copy2(HAPROXY_CONFIG_PATH, HAPROXY_BACKUP_PATH) - if os.path.exists(BLOCKED_IPS_MAP_PATH): - shutil.copy2(BLOCKED_IPS_MAP_PATH, BLOCKED_IPS_MAP_BACKUP_PATH) - logger.info("Backups created successfully") - return True + if os.path.getsize(path_a) != os.path.getsize(path_b): + return False + with open(path_a, 'rb') as fa, open(path_b, 'rb') as fb: + while True: + chunk_a = fa.read(65536) + chunk_b = fb.read(65536) + if chunk_a != chunk_b: + return False + if not chunk_a: + return True + except OSError: + return False + + +def _config_set_matches_backup(): + """True if every live config file is byte-identical to its backup copy. + + After a successful reload the live set has already been recorded as + known-good (see promote_current_config_to_backup()), which is the common + case at the start of the next generation. Recognising it lets create_backup() + skip both the re-validation and the copy - worth doing because + `haproxy -c` on an edge with hundreds of certificates is not free and + generate_config() runs synchronously inside customer-facing API calls. + """ + for live_path, backup_path in _config_backup_pairs(): + if os.path.exists(live_path) != os.path.exists(backup_path): + return False + if (os.path.exists(live_path) + and not _files_identical(live_path, backup_path)): + return False + return True + + +def _config_backup_pairs(): + """(live, backup) pairs forming one restorable config set. + + Built at call time rather than at import so the module-level path constants + stay patchable (tests, alternate deployments). + """ + return ( + (HAPROXY_CONFIG_PATH, HAPROXY_BACKUP_PATH), + (BLOCKED_IPS_MAP_PATH, BLOCKED_IPS_MAP_BACKUP_PATH), + (CORAZA_SPOE_CONFIG_PATH, CORAZA_SPOE_BACKUP_PATH), + ) + + +def write_config_atomically(path, content): + """Write content to path via temp file + rename. + + A half-written haproxy.cfg (disk full, container killed mid-write) is just + as fatal as an invalid one and is invisible to the caller. os.replace() is + atomic within a filesystem, so the file on disk is always either the whole + old config or the whole new one — never a truncated hybrid. This also keeps + the "existing config is already broken" case from being self-inflicted. + """ + directory = os.path.dirname(path) or '.' + # Preserve the mode of the file we are replacing; mkstemp defaults to 0600 + # and HAProxy config files are conventionally 0644. + try: + mode = stat.S_IMODE(os.stat(path).st_mode) + except OSError: + mode = 0o644 + fd, tmp_path = tempfile.mkstemp( + dir=directory, prefix=os.path.basename(path) + '.', suffix='.tmp' + ) + try: + with os.fdopen(fd, 'w') as f: + f.write(content) + f.flush() + os.fsync(f.fileno()) + os.chmod(tmp_path, mode) + os.replace(tmp_path, path) + except Exception: + try: + os.unlink(tmp_path) + except OSError: + pass + raise + + +def create_backup(require_valid=True): + """Snapshot the CURRENT on-disk config set as the rollback point. + + MUST be called BEFORE the new configuration is written — see the module + comment above. Calling it afterwards silently disarms rollback. + + require_valid=True (default) refuses to promote a config that HAProxy + already rejects. Backing up a broken config would make "rollback" mean + "restore a different broken config"; keeping the older, validated backup + instead means a rollback always lands on something HAProxy will actually + start with. Cost is one `haproxy -c` run per config generation. + + Returns (ok, status): + ok=False, status='error' - the copy itself failed; caller decides. + status='created' - backup now holds the current config. + status='kept_previous' - current config missing or invalid; the + existing (older, good) backup was kept. + status='unavailable' - nothing to roll back to at all (first + run, or broken config and no prior + backup). Rollback is NOT possible. + """ + try: + snapshot_ok = True + reason = None + + if not os.path.exists(HAPROXY_CONFIG_PATH): + snapshot_ok = False + reason = 'no existing HAProxy config on disk (first run?)' + elif _config_set_matches_backup(): + # The backup already IS the current config, recorded when it last + # loaded successfully. Nothing to copy and nothing to re-validate. + logger.debug("Config backup already matches the live config") + return True, 'created' + elif require_valid: + status, msg = validate_config_file(HAPROXY_CONFIG_PATH) + if status == 'invalid': + snapshot_ok = False + reason = f'current config on disk does not validate: {msg}' + elif status == 'unavailable': + # The validator itself could not run (no haproxy binary, etc). + # That is NOT evidence the config is bad, and refusing to back + # up would leave us with no rollback target at all, so fall + # back to last-written semantics and say so loudly. + logger.warning( + f"Could not verify current config before backup ({msg}); " + "backing it up unverified" + ) + + if not snapshot_ok: + if os.path.exists(HAPROXY_BACKUP_PATH): + logger.warning( + f"Not refreshing config backup: {reason}. Keeping the " + f"existing backup at {HAPROXY_BACKUP_PATH} as the rollback " + "target." + ) + return True, 'kept_previous' + logger.error( + f"No config backup could be taken: {reason}, and no previous " + f"backup exists at {HAPROXY_BACKUP_PATH}. ROLLBACK IS NOT " + "AVAILABLE for this configuration change." + ) + return True, 'unavailable' + + for live_path, backup_path in _config_backup_pairs(): + if os.path.exists(live_path): + shutil.copy2(live_path, backup_path) + logger.info("Backup of last-known-good config created successfully") + return True, 'created' except Exception as e: logger.error(f"Failed to create backup: {e}") - return False + return False, 'error' -def restore_backup(): - """Restore from backup files""" +def promote_current_config_to_backup(): + """Record the live config as the known-good rollback target. + + Called ONLY after the config has both validated and been loaded by HAProxy, + so "backup" really means "the last configuration this box was running". + Must never be called before a reload attempt: doing so would make the + backup a copy of the config we may still have to roll back from - the same + class of bug as backing up after the write. + + Without this, a box whose very first generation succeeded has no rollback + target at all until its second successful generation, and any corruption of + haproxy.cfg in between leaves nothing to recover to. + """ try: - if os.path.exists(HAPROXY_BACKUP_PATH): - shutil.copy2(HAPROXY_BACKUP_PATH, HAPROXY_CONFIG_PATH) - if os.path.exists(BLOCKED_IPS_MAP_BACKUP_PATH): - shutil.copy2(BLOCKED_IPS_MAP_BACKUP_PATH, BLOCKED_IPS_MAP_PATH) - logger.info("Backups restored successfully") + for live_path, backup_path in _config_backup_pairs(): + if os.path.exists(live_path): + shutil.copy2(live_path, backup_path) + logger.debug("Known-good config backup updated after successful reload") return True except Exception as e: - logger.error(f"Failed to restore backup: {e}") + # Non-fatal: the config is live and working, we just failed to record + # it. Loud, because the next change now has a staler rollback target. + logger.error(f"Failed to record known-good config backup: {e}") return False -def validate_haproxy_config(): - """Validate HAProxy configuration file""" - try: - result = subprocess.run(['haproxy', '-c', '-f', HAPROXY_CONFIG_PATH], - capture_output=True, text=True) - if result.returncode == 0: - logger.info("HAProxy configuration validation passed") - return True, None - else: - error_msg = f"HAProxy configuration validation failed: {result.stderr}" - logger.error(error_msg) - return False, error_msg - except Exception as e: - error_msg = f"Error validating HAProxy config: {e}" - logger.error(error_msg) - return False, error_msg -def reload_haproxy_safely(): - """Safely reload HAProxy with validation and rollback""" +def restore_backup(): + """Restore the backed-up config set over the live files. + + Returns (restored, message). restored=False means NOTHING was rolled back + and the live config is still whatever the failed change left on disk — + callers MUST surface that difference, it is the difference between "we + recovered" and "this edge is sitting on a config HAProxy will not load". + """ + if not os.path.exists(HAPROXY_BACKUP_PATH): + msg = (f"No config backup at {HAPROXY_BACKUP_PATH} - cannot roll back; " + f"{HAPROXY_CONFIG_PATH} still holds the failed configuration") + logger.critical(msg) + return False, msg try: - # Create backup before changes - if not create_backup(): - return False, "Failed to create backup" - + for live_path, backup_path in _config_backup_pairs(): + if os.path.exists(backup_path): + shutil.copy2(backup_path, live_path) + msg = f"Configuration restored from backup ({HAPROXY_BACKUP_PATH})" + logger.info(msg) + return True, msg + except Exception as e: + msg = (f"Failed to restore backup: {e} - {HAPROXY_CONFIG_PATH} may hold " + "a broken configuration") + logger.critical(msg) + return False, msg + + +def validate_config_file(config_path): + """Run `haproxy -c` against config_path. + + Returns (status, message) with status one of: + 'valid' - HAProxy parsed the file successfully + 'invalid' - HAProxy rejected it (message carries stderr) + 'unavailable' - the validator could not be run at all (binary missing, + timeout, ...). Deliberately distinct from 'invalid': + it tells us nothing about the config. + """ + try: + result = subprocess.run(['haproxy', '-c', '-f', config_path], + capture_output=True, text=True) + except Exception as e: + return 'unavailable', f"Error validating HAProxy config: {e}" + if result.returncode == 0: + return 'valid', None + return 'invalid', f"HAProxy configuration validation failed: {result.stderr}" + + +def validate_haproxy_config(): + """Validate the live HAProxy configuration file. Returns (is_valid, error).""" + status, message = validate_config_file(HAPROXY_CONFIG_PATH) + if status == 'valid': + logger.info("HAProxy configuration validation passed") + return True, None + logger.error(message) + return False, message + +def reload_haproxy_safely(backup_status=None): + """Safely reload HAProxy with validation and rollback. + + PRECONDITION: the caller must already have called create_backup() BEFORE + writing the new config, and pass the status it returned. This function runs + after the new config is on disk, so it cannot take a meaningful backup + itself — doing so is exactly the bug this contract exists to prevent. + + backup_status=None means the caller did not take a pre-write backup. We do + NOT create one here (that would overwrite a genuinely good backup with the + unverified new config); we log it and fall back to whatever backup already + exists on disk. + """ + try: + if backup_status is None: + logger.error( + "reload_haproxy_safely() called without a pre-write backup " + "status - rollback will fall back to whatever backup already " + "exists on disk. Callers must call create_backup() BEFORE " + "writing the new configuration." + ) + elif backup_status not in _ROLLBACK_AVAILABLE_STATUSES: + logger.warning( + f"Proceeding with reload without a rollback target " + f"(backup status: {backup_status})" + ) + # Validate new configuration is_valid, error_msg = validate_haproxy_config() if not is_valid: # Restore backup on validation failure - restore_backup() + restored, restore_msg = restore_backup() + if not restored: + logger.critical( + "Config validation failed AND rollback was not possible - " + f"{HAPROXY_CONFIG_PATH} holds an invalid configuration that " + "HAProxy will refuse to start with" + ) + return False, (f"Config validation failed: {error_msg} | " + f"ROLLBACK FAILED: {restore_msg}") return False, f"Config validation failed: {error_msg}" - + # Attempt reload if is_process_running('haproxy'): # Use HAProxy stats socket for graceful reload @@ -2094,20 +2355,28 @@ def reload_haproxy_safely(): if reload_result.returncode == 0: logger.info("HAProxy reloaded successfully") + # Now - and only now - is this config known good. + promote_current_config_to_backup() return True, "HAProxy reloaded successfully" else: # Reload failed, restore backup - restore_backup() - # Try to reload with backup config - subprocess.run('echo "reload" | socat stdio /tmp/haproxy-cli', - shell=True, capture_output=True) + restored, restore_msg = restore_backup() + if restored: + # Try to reload with the restored (known-good) config + subprocess.run( + 'echo "reload" | socat stdio /tmp/haproxy-cli', + shell=True, capture_output=True) error_msg = f"HAProxy reload failed: {reload_result.stderr}" + if not restored: + error_msg += f" | ROLLBACK FAILED: {restore_msg}" logger.error(error_msg) return False, error_msg except Exception as e: # Critical error during reload, restore backup - restore_backup() + restored, restore_msg = restore_backup() error_msg = f"Critical error during reload: {e}" + if not restored: + error_msg += f" | ROLLBACK FAILED: {restore_msg}" logger.error(error_msg) return False, error_msg else: @@ -2118,11 +2387,15 @@ def reload_haproxy_safely(): check=True, capture_output=True, text=True ) logger.info("HAProxy started successfully") + # Now - and only now - is this config known good. + promote_current_config_to_backup() return True, "HAProxy started successfully" except subprocess.CalledProcessError as e: # Start failed, restore backup - restore_backup() + restored, restore_msg = restore_backup() error_msg = f"Failed to start HAProxy: {e.stderr}" + if not restored: + error_msg += f" | ROLLBACK FAILED: {restore_msg}" logger.error(error_msg) return False, error_msg except Exception as e: diff --git a/scripts/test-config-rollback.py b/scripts/test-config-rollback.py new file mode 100755 index 0000000..ad84b97 --- /dev/null +++ b/scripts/test-config-rollback.py @@ -0,0 +1,467 @@ +#!/usr/bin/env python3 +"""Regression tests for HAProxy config backup / rollback ordering. + +Why this file exists +-------------------- +generate_config() used to write the new haproxy.cfg and only THEN call +reload_haproxy_safely() -> create_backup(), so the "backup" was a copy of the +config that had just been written. On a validation failure restore_backup() +restored the identical broken bytes: the advertised rollback was a no-op and a +fatal haproxy.cfg stayed on disk, where start_haproxy() refuses to launch. + +These tests pin the ordering invariant (backup predates the write) and the +observable end-to-end behaviour (after a failed validation the file on disk is +the previous working config and HAProxy will start with it). + +Running +------- + python3 scripts/test-config-rollback.py # tests the repo checkout + HAPROXY_MANAGER_DIR=/some/other/tree \ + python3 scripts/test-config-rollback.py # tests another tree + +The repo has no Python test framework (scripts/test-*.sh are curl-based +integration scripts against a running API), so this is a self-contained +stdlib-unittest script - no pytest, no venv, no new dependencies beyond the +application's own requirements.txt (Flask/Jinja2/psutil), which are already +present in the container image. + +No HAProxy binary is required: a stub `haproxy` is put on PATH that mimics +`haproxy -c -f ` by rejecting any config containing the token +__BROKEN__, which is how the tests inject an invalid configuration. +""" + +import os +import sys +import shutil +import sqlite3 +import logging +import tempfile +import textwrap +import unittest + +BROKEN_TOKEN = '__BROKEN__' + +MODULE_DIR = os.path.abspath( + os.environ.get('HAPROXY_MANAGER_DIR', + os.path.join(os.path.dirname(os.path.abspath(__file__)), '..')) +) + +# haproxy_manager builds its Jinja2 environment from the relative path +# Path('templates'), so it has to be imported with the module dir as cwd. +os.chdir(MODULE_DIR) +sys.path.insert(0, MODULE_DIR) + +# The module opens /var/log/haproxy-manager.log at import time via +# logging.FileHandler. Redirect that one call so the suite runs unprivileged. +_LOG_DIR = tempfile.mkdtemp(prefix='haproxy-mgr-test-logs-') +_real_file_handler = logging.FileHandler +logging.FileHandler = ( + lambda fn, *a, **kw: _real_file_handler( + os.path.join(_LOG_DIR, os.path.basename(fn)), *a, **kw) +) +try: + import haproxy_manager as hm +except ImportError as exc: # pragma: no cover - environment problem, not a failure + sys.stderr.write( + f"SKIP: cannot import haproxy_manager ({exc}).\n" + "Install the application requirements first: pip install -r requirements.txt\n" + ) + raise SystemExit(77) +finally: + logging.FileHandler = _real_file_handler + +logging.getLogger('haproxy_manager').setLevel(logging.CRITICAL) + +FAKE_HAPROXY = textwrap.dedent(f"""\ + #!/bin/sh + # Test stub for the haproxy binary. + # haproxy -c -f FILE -> exit 1 if FILE contains {BROKEN_TOKEN}, else 0 + # haproxy -W -S ... -f FILE (start) -> same validation, then exit 0 + cfg="" + while [ $# -gt 0 ]; do + case "$1" in -f) cfg="$2"; shift ;; esac + shift + done + if [ -n "$cfg" ] && grep -q '{BROKEN_TOKEN}' "$cfg" 2>/dev/null; then + echo "[ALERT] parsing [$cfg:1] : unknown keyword '{BROKEN_TOKEN}'" >&2 + exit 1 + fi + exit 0 +""") + + +class RollbackTestCase(unittest.TestCase): + """Base fixture: an isolated fake /etc/haproxy plus a stub haproxy binary.""" + + def setUp(self): + self.tmp = tempfile.mkdtemp(prefix='haproxy-rollback-test-') + self.addCleanup(shutil.rmtree, self.tmp, True) + + bindir = os.path.join(self.tmp, 'bin') + os.makedirs(bindir) + stub = os.path.join(bindir, 'haproxy') + with open(stub, 'w') as fh: + fh.write(FAKE_HAPROXY) + os.chmod(stub, 0o755) + self._old_path = os.environ['PATH'] + os.environ['PATH'] = bindir + os.pathsep + self._old_path + self.addCleanup(lambda: os.environ.__setitem__('PATH', self._old_path)) + + self.etc = os.path.join(self.tmp, 'etc') + os.makedirs(self.etc) + + overrides = { + 'DB_FILE': os.path.join(self.etc, 'haproxy_config.db'), + 'HAPROXY_CONFIG_PATH': os.path.join(self.etc, 'haproxy.cfg'), + 'HAPROXY_BACKUP_PATH': os.path.join(self.etc, 'haproxy.cfg.backup'), + 'BLOCKED_IPS_MAP_PATH': os.path.join(self.etc, 'blocked_ips.map'), + 'BLOCKED_IPS_MAP_BACKUP_PATH': os.path.join(self.etc, 'blocked_ips.map.backup'), + 'CLUSTER_SECRET_PATH': os.path.join(self.etc, 'cluster-secret'), + 'SSL_CERTS_DIR': os.path.join(self.etc, 'certs'), + 'HAPROXY_SOCKET_PATH': os.path.join(self.etc, 'haproxy.sock'), + # Added by the rollback fix; older trees do not have it. + 'CORAZA_SPOE_CONFIG_PATH': os.path.join(self.etc, 'coraza-spoe.cfg'), + 'CORAZA_SPOE_BACKUP_PATH': os.path.join(self.etc, 'coraza-spoe.cfg.backup'), + } + self._saved = {} + for name, value in overrides.items(): + self._saved[name] = getattr(hm, name, None) + setattr(hm, name, value) + self.addCleanup(self._restore_globals) + os.makedirs(hm.SSL_CERTS_DIR) + + # log_operation() appends to a hardcoded /var/log path. Injecting `open` + # into the module namespace shadows the builtin for that module only + # (module globals are searched before builtins), so the real + # log_operation code still runs. + real_open = open + log_dir = self.tmp + + def _redirecting_open(path, *args, **kwargs): + if isinstance(path, str) and path.startswith('/var/log/'): + path = os.path.join(log_dir, os.path.basename(path)) + return real_open(path, *args, **kwargs) + + hm.open = _redirecting_open + self.addCleanup(lambda: hm.__dict__.pop('open', None)) + + hm.init_db() + + def _restore_globals(self): + for name, value in self._saved.items(): + if value is None: + hm.__dict__.pop(name, None) + else: + setattr(hm, name, value) + + # -- helpers --------------------------------------------------------- + def add_domain(self, domain, backend_name, address='10.0.0.1'): + with sqlite3.connect(hm.DB_FILE) as conn: + cur = conn.cursor() + cur.execute('INSERT INTO domains (domain, ssl_enabled) VALUES (?, 0)', + (domain,)) + domain_id = cur.lastrowid + cur.execute('INSERT INTO backends (name, domain_id) VALUES (?, ?)', + (backend_name, domain_id)) + backend_id = cur.lastrowid + cur.execute( + 'INSERT INTO backend_servers ' + '(backend_id, server_name, server_address, server_port) ' + 'VALUES (?, ?, ?, ?)', + (backend_id, 'srv1', address, 8080)) + conn.commit() + + def block_ip(self, ip): + with sqlite3.connect(hm.DB_FILE) as conn: + conn.execute('INSERT INTO blocked_ips (ip_address, reason) VALUES (?, ?)', + (ip, 'test')) + conn.commit() + + def read(self, path): + with open(path) as fh: + return fh.read() + + def config_is_loadable(self): + """True if HAProxy would accept the config currently on disk.""" + import subprocess + return subprocess.run( + ['haproxy', '-c', '-f', hm.HAPROXY_CONFIG_PATH], + capture_output=True).returncode == 0 + + def generate_good_config(self): + self.add_domain('good.example.com', 'good_backend') + hm.generate_config() + self.assertTrue(self.config_is_loadable(), + 'fixture precondition: first generated config must be valid') + return self.read(hm.HAPROXY_CONFIG_PATH) + + def break_the_config(self): + """Queue a domain whose rendered backend the validator rejects.""" + self.add_domain('bad.example.com', BROKEN_TOKEN + '_backend', '10.0.0.2') + + +class TestBackupOrdering(RollbackTestCase): + + def test_backup_is_taken_before_the_new_config_is_written(self): + """The ordering invariant, asserted directly. + + Whatever create_backup() sees on disk must be the OLD config; if the + write happens first the backup is a copy of the new config and rollback + is meaningless. + """ + good = self.generate_good_config() + + seen = {} + real_create_backup = hm.create_backup + + def spy(*args, **kwargs): + seen['config_on_disk'] = self.read(hm.HAPROXY_CONFIG_PATH) + return real_create_backup(*args, **kwargs) + + hm.create_backup = spy + self.addCleanup(setattr, hm, 'create_backup', real_create_backup) + + self.add_domain('second.example.com', 'second_backend', '10.0.0.3') + hm.generate_config() + + self.assertIn('config_on_disk', seen, + 'create_backup() was never called during generate_config()') + self.assertEqual( + seen['config_on_disk'], good, + 'create_backup() ran AFTER the new config was written - the backup ' + 'is a copy of the new config, so rollback cannot undo anything') + + def test_backup_tracks_the_last_known_good_config(self): + """After a change that validated AND loaded, the backup is that config. + + The rollback target is "the last configuration HAProxy actually ran", + not "the file that happened to be there last time". + """ + good = self.generate_good_config() + self.add_domain('second.example.com', 'second_backend', '10.0.0.3') + hm.generate_config() + + live = self.read(hm.HAPROXY_CONFIG_PATH) + self.assertNotEqual(live, good, 'fixture sanity: the new config should differ') + self.assertEqual(self.read(hm.HAPROXY_BACKUP_PATH), live, + 'the successful config was not recorded as known-good') + + def test_backup_is_not_promoted_when_the_change_fails(self): + """A config that never loaded must not become the rollback target.""" + good = self.generate_good_config() + self.break_the_config() + with self.assertRaises(Exception): + hm.generate_config() + + self.assertEqual(self.read(hm.HAPROXY_BACKUP_PATH), good, + 'a config that failed validation was promoted to backup') + + +class TestRollbackEndToEnd(RollbackTestCase): + + def test_failed_validation_leaves_the_last_good_config_on_disk(self): + good = self.generate_good_config() + + self.break_the_config() + with self.assertRaises(Exception): + hm.generate_config() + + on_disk = self.read(hm.HAPROXY_CONFIG_PATH) + self.assertNotIn(BROKEN_TOKEN, on_disk, + 'the rejected config is still on disk - rollback was a no-op') + self.assertEqual(on_disk, good, + 'on-disk config is not byte-identical to the last good one') + + def test_haproxy_would_still_start_after_a_failed_change(self): + """The operational consequence: the edge can still come up.""" + self.generate_good_config() + self.break_the_config() + with self.assertRaises(Exception): + hm.generate_config() + + self.assertTrue(self.config_is_loadable(), + 'HAProxy would refuse to start with the config left on disk') + with self.assertLogs('haproxy_manager', level='INFO') as captured: + hm.start_haproxy() + self.assertTrue( + any('HAProxy started successfully' in line for line in captured.output), + f'start_haproxy() did not succeed after rollback: {captured.output}') + + def test_blocked_ips_map_is_rolled_back_too(self): + """generate_config() rewrites the map file before writing haproxy.cfg.""" + self.block_ip('192.0.2.10') + self.generate_good_config() + good_map = self.read(hm.BLOCKED_IPS_MAP_PATH) + + self.block_ip('198.51.100.20') + self.break_the_config() + with self.assertRaises(Exception): + hm.generate_config() + + self.assertEqual(self.read(hm.BLOCKED_IPS_MAP_PATH), good_map, + 'blocked IPs map was not rolled back with the config') + + def test_first_run_failure_reports_that_rollback_was_impossible(self): + """No prior config: there is nothing to restore, and that must be said. + + A missing backup must never be reported as a successful restore, and it + must never be turned into "restore an empty file". + """ + self.break_the_config() + with self.assertRaises(Exception) as ctx: + hm.generate_config() + + self.assertIn('ROLLBACK FAILED', str(ctx.exception), + 'a failed change with no backup was not reported as such') + self.assertFalse(os.path.exists(hm.HAPROXY_BACKUP_PATH), + 'a backup was fabricated from the broken config') + # The broken config is deliberately left in place: start_haproxy() can + # then detect it and try to regenerate. It must not be blanked. + self.assertGreater(os.path.getsize(hm.HAPROXY_CONFIG_PATH), 0, + 'config file was emptied instead of left for diagnosis') + + +class TestBackupPrimitives(RollbackTestCase): + + def test_restore_backup_distinguishes_missing_backup_from_success(self): + restored, message = hm.restore_backup() + self.assertFalse(restored, + 'restore_backup() reported success with no backup present') + self.assertIn('cannot roll back', message.lower()) + + good = self.generate_good_config() + with open(hm.HAPROXY_CONFIG_PATH, 'w') as fh: + fh.write('scribbled over\n') + + restored, message = hm.restore_backup() + self.assertTrue(restored, message) + self.assertEqual(self.read(hm.HAPROXY_CONFIG_PATH), good) + + def test_a_successful_generation_records_a_rollback_target(self): + """Even the first-ever generation must leave something to roll back to.""" + good = self.generate_good_config() + self.assertTrue( + os.path.exists(hm.HAPROXY_BACKUP_PATH), + 'after a successful reload there is still no known-good backup') + self.assertEqual(self.read(hm.HAPROXY_BACKUP_PATH), good) + + def test_a_broken_current_config_does_not_replace_a_good_backup(self): + """The known-good marker. + + If the config already on disk is broken (previous failed write, manual + edit), snapshotting it would make "rollback" mean "restore a different + broken config". The older validated backup must survive. + """ + good = self.generate_good_config() + self.assertEqual(self.read(hm.HAPROXY_BACKUP_PATH), good, + 'fixture: a good backup should exist by now') + + with open(hm.HAPROXY_CONFIG_PATH, 'w') as fh: + fh.write(f'garbage {BROKEN_TOKEN} config\n') + + ok, status = hm.create_backup() + self.assertTrue(ok) + self.assertEqual(status, 'kept_previous') + self.assertEqual(self.read(hm.HAPROXY_BACKUP_PATH), good, + 'a broken config overwrote the known-good backup') + + def test_reload_does_not_take_its_own_backup(self): + """reload_haproxy_safely() runs after the write, so it must not back up.""" + good = self.generate_good_config() + with open(hm.HAPROXY_CONFIG_PATH, 'w') as fh: + fh.write(f'broken {BROKEN_TOKEN}\n') + + success, message = hm.reload_haproxy_safely(backup_status='created') + + self.assertFalse(success) + self.assertEqual(self.read(hm.HAPROXY_BACKUP_PATH), good, + 'reload_haproxy_safely() overwrote the good backup') + self.assertEqual(self.read(hm.HAPROXY_CONFIG_PATH), good, + 'reload_haproxy_safely() did not roll the config back') + + def test_unchanged_config_is_not_revalidated(self): + """Fast path: if the backup already is the live config, do no work. + + generate_config() runs inside customer-facing API calls and + `haproxy -c` is expensive on an edge with hundreds of certificates. + """ + self.generate_good_config() + + calls = [] + real_validate = hm.validate_config_file + hm.validate_config_file = lambda path: (calls.append(path), + real_validate(path))[1] + self.addCleanup(setattr, hm, 'validate_config_file', real_validate) + + ok, status = hm.create_backup() + self.assertTrue(ok) + self.assertEqual(status, 'created') + self.assertEqual(calls, [], + 'the unchanged live config was re-validated needlessly') + + def test_fast_path_does_not_hide_a_drifted_broken_config(self): + """If the live config drifted from the backup, the gate must still run.""" + good = self.generate_good_config() + with open(hm.HAPROXY_CONFIG_PATH, 'w') as fh: + fh.write(f'hand edited {BROKEN_TOKEN}\n') + + ok, status = hm.create_backup() + self.assertTrue(ok) + self.assertEqual(status, 'kept_previous', + 'a drifted broken config was silently accepted') + self.assertEqual(self.read(hm.HAPROXY_BACKUP_PATH), good) + + def test_backup_set_covers_every_file_generate_config_writes(self): + pairs = dict(hm._config_backup_pairs()) + for path in (hm.HAPROXY_CONFIG_PATH, hm.BLOCKED_IPS_MAP_PATH, + hm.CORAZA_SPOE_CONFIG_PATH): + self.assertIn(path, pairs, + f'{path} is written by generate_config() but is not ' + 'part of the backed-up config set') + + def test_coraza_spoe_config_round_trips(self): + self.generate_good_config() + with open(hm.CORAZA_SPOE_CONFIG_PATH, 'w') as fh: + fh.write('spoe-good\n') + hm.create_backup() + with open(hm.CORAZA_SPOE_CONFIG_PATH, 'w') as fh: + fh.write('spoe-broken\n') + restored, message = hm.restore_backup() + self.assertTrue(restored, message) + self.assertEqual(self.read(hm.CORAZA_SPOE_CONFIG_PATH), 'spoe-good\n') + + +class TestAtomicWrite(RollbackTestCase): + + def test_write_is_atomic_and_preserves_mode(self): + path = os.path.join(self.etc, 'atomic.cfg') + with open(path, 'w') as fh: + fh.write('old') + os.chmod(path, 0o644) + + hm.write_config_atomically(path, 'new content\n') + + self.assertEqual(self.read(path), 'new content\n') + self.assertEqual(oct(os.stat(path).st_mode & 0o777), oct(0o644)) + leftovers = [n for n in os.listdir(self.etc) if n.endswith('.tmp')] + self.assertEqual(leftovers, [], f'temp files left behind: {leftovers}') + + def test_failed_write_leaves_the_previous_file_intact(self): + path = os.path.join(self.etc, 'atomic.cfg') + with open(path, 'w') as fh: + fh.write('old content\n') + + # Anything that makes f.write() blow up mid-flight stands in for a full + # disk / killed container. + with self.assertRaises(Exception): + hm.write_config_atomically(path, object()) + + self.assertEqual(self.read(path), 'old content\n', + 'a failed write clobbered the previous config') + leftovers = [n for n in os.listdir(self.etc) if n.endswith('.tmp')] + self.assertEqual(leftovers, [], f'temp files left behind: {leftovers}') + + +if __name__ == '__main__': + print(f"testing haproxy_manager from: {MODULE_DIR}") + unittest.main(verbosity=2) From 233044fb1d6f2fe5c4abe2c0a0428050cab77fb9 Mon Sep 17 00:00:00 2001 From: Josh Knapp Date: Thu, 6 Aug 2026 09:45:35 -0700 Subject: [PATCH 2/4] fix(config): write blocked_ips.map atomically; close review gaps Follow-up to 9d16151 (take the rollback backup BEFORE writing the new config). Review findings, none of them blockers, plus two corrections to the record. blocked_ips.map was still written with a plain open(path,'w'). `haproxy -c` LOADS that file - hap_listener.tpl matches on map_ip(/etc/haproxy/blocked_ips.map,0) - and a half-written final line is a FATAL config error, not one dropped entry. Verified against HAProxy 2.8: a truncated "198.51.10" gives "is not a valid IPv4 or IPv6 address at line 2 of file ..." and the whole configuration is rejected. That is precisely the failure shape the backup-ordering fix exists to prevent, on a different file, reachable from all five /api/blocked-ips routes. It now goes through the same write_config_atomically() as haproxy.cfg and coraza-spoe.cfg. (A truncation that happens to land on a line boundary is not fatal - it silently drops blocks - which is its own reason to write the file atomically.) The previous commit message claimed the fast path adds "no `haproxy -c` latency to customer-facing API calls". That was measured on an idle box and is false in this fleet's normal pattern: update_blocked_ips_map() is called from those five routes OUTSIDE generate_config(), so the live map drifts from its backup and the NEXT config change misses create_backup()'s fast path. Measured `haproxy -c` runs per domain add: 1 steady state, 2 after an IP block. This fleet blocks IPs automatically, so the 2x recurred on the customer-facing call indefinitely. update_blocked_ips_map() now promotes the map it just wrote to its backup, restoring the steady state - guarded twice: nothing is promoted unless a config backup set already exists (never fabricate a rollback target), and not unless the map parses as IPs/CIDRs, so promotion cannot leave a "rollback target" HAProxy would refuse to load. generate_config() passes promote_backup=False: it took the snapshot moments earlier and the map is part of the not-yet-validated change, so refreshing the backup there would be the original bug again. The remaining 2 is the first generation after this upgrade (coraza-spoe.cfg has no backup yet); that is once per host, by construction. Two claims in 9d16151's message are wrong and are corrected here rather than by rewriting a pushed commit: * "12 of the 17 fail against the previous code" - it is 14 of 17 (6 failures + 8 errors). 12 was measured before two fast-path tests were added and never re-measured. * "a missing haproxy binary is not read as a bad config" - true of create_backup() only. validate_haproxy_config() collapses both 'invalid' and 'unavailable' to False, so in the reload path a missing validator still triggers a full rollback labelled "Config validation failed". The behaviour is right (without a working validator we cannot claim the new config is safe, and the reload path is where guessing wrong takes the edge down); the sentence was broader than the code. Now documented on the function. Tests: 26 (was 17), all green; 22 fail against main. New coverage closes the review's mutation survivors: * the "Refusing to regenerate config" guard, previously entirely uncovered; * the invalid/unavailable split, previously zero coverage - both the verdict and the consequence create_backup() draws from it; * the byte-compare loop, with a same-size-different-content config, which is the exact case the "not filecmp.cmp" rationale exists for. Building that fixture found a bug in the test itself: sizing the drifted config with len(str) instead of bytes made it pass for the wrong reason, because the rendered config contains non-ASCII. * test_failed_write_leaves_the_previous_file_intact was vacuous: its bare assertRaises(Exception) swallowed the AttributeError from write_config_atomically not existing, so it passed against main and would have kept passing if the function were deleted. Narrowed to TypeError - proven by deleting the function and watching it go red. * test_backup_set_covers_every_file_generate_config_writes restated the three files it expected, so it could never have noticed a fourth. It now derives the set - observed on disk for the branches the fixture can execute, read out of generate_config()'s source for the env-gated one that writes a hardcoded /etc/haproxy path - and requires anything unbacked-up to be on a documented exclusion list (suspended_domains.list, cluster-secret, each with its reason). Proven by adding a fourth written file and watching it fail. Every change above was mutation-proved: 11 mutations, 0 survivors, each reddening only the tests that cover it. Two review items were confirmed untestable in-process and are deliberately skipped: the fsync (M11) and a log-line-only mutation (M15). Left alone deliberately, all pre-existing on main and unchanged here: the outer `except Exception` in reload_haproxy_safely() does not roll back (narrow window, now commented at the site); stale .tmp files after SIGKILL are never swept (verified inert - nothing globs /etc/haproxy, and the only directory-wide load is `crt /etc/haproxy/certs`, which nothing here writes to); a partial backup-copy failure can leave a mixed-vintage backup set (very narrow, and generate_config() correctly refuses to write). VERSION stays 2026.08.1. NOTE: fix/cert-write-safety, which is stacked on this branch, carries the same 2026.08.1. If both land on main as separate commits, CI pushes :2026.08.1 twice with different content. Either that branch moves to 2026.08.2 or the two land as a single merge - not decided here. No template, QUIC or HTTP/3 changes. Co-Authored-By: Claude Opus 5 (1M context) --- haproxy_manager.py | 119 +++++++++++- scripts/test-config-rollback.py | 324 +++++++++++++++++++++++++++++++- 2 files changed, 427 insertions(+), 16 deletions(-) diff --git a/haproxy_manager.py b/haproxy_manager.py index b7ef7ef..7390cd6 100644 --- a/haproxy_manager.py +++ b/haproxy_manager.py @@ -1853,8 +1853,12 @@ def generate_config(): ) config_parts.append(default_headers) - # Update blocked IPs map file first - update_blocked_ips_map() + # Update blocked IPs map file first. promote_backup=False: the rollback + # snapshot was taken a few lines above and this map is part of the + # not-yet-validated change, so refreshing the backup copy here would + # overwrite the bytes rollback needs - the same class of bug as backing + # up after the write. + update_blocked_ips_map(promote_backup=False) # Add Listener Block listener_block = template_env.get_template('hap_listener.tpl').render( @@ -2287,7 +2291,19 @@ def validate_config_file(config_path): def validate_haproxy_config(): - """Validate the live HAProxy configuration file. Returns (is_valid, error).""" + """Validate the live HAProxy configuration file. Returns (is_valid, error). + + NOTE the invalid/unavailable distinction validate_config_file() draws does + NOT survive here: this returns a bare bool, so a validator that could not + run is reported the same as a rejected config and the reload path rolls + back, logging "Config validation failed". That is deliberate - without a + working validator we cannot claim the new config is safe, and the reload + path is the one place where guessing wrong takes the edge down. The + distinction is only acted on inside create_backup(), where treating + "cannot check" as "bad" would mean refusing to keep any rollback target at + all. (The 2026-08 commit message said flatly that "a missing haproxy binary + is not read as a bad config"; that is true of create_backup() only.) + """ status, message = validate_config_file(HAPROXY_CONFIG_PATH) if status == 'valid': logger.info("HAProxy configuration validation passed") @@ -2399,12 +2415,89 @@ def reload_haproxy_safely(backup_status=None): logger.error(error_msg) return False, error_msg except Exception as e: + # KNOWN GAP (pre-existing, unchanged by the 2026-08 backup-ordering + # fix): this outer handler does NOT roll back. The window is narrow - + # everything after the validation gate has its own handler - but an + # exception raised between the gate and those handlers leaves the new + # config on disk. Left as-is deliberately; rolling back from here would + # also undo changes that had in fact loaded. error_msg = f"Critical error in reload process: {e}" logger.error(error_msg) return False, error_msg -def update_blocked_ips_map(): - """Update the blocked IPs map file from database""" +def _blocked_ips_map_is_wellformed(path): + """True if every key in a blocked-IPs map is one HAProxy will parse. + + map_ip() rejects the ENTIRE configuration if a single key is not an IP or + CIDR ("'198.51.10' is not a valid IPv4 or IPv6 address at line 2 of file + ..."), so this is the property that decides whether the file is safe to + record as a rollback target. Pure Python over a small file - deliberately + not another `haproxy -c`, which is the cost this check exists to avoid. + """ + try: + with open(path) as fh: + for line in fh: + line = line.strip() + if not line or line.startswith('#'): + continue + ipaddress.ip_network(line.split()[0], strict=False) + except (OSError, ValueError, IndexError): + return False + return True + + +def _promote_blocked_ips_map_to_backup(): + """Keep blocked_ips.map.backup in step with the map just written. + + The /api/blocked-ips routes call update_blocked_ips_map() OUTSIDE + generate_config(): the map write IS the whole change there, published + against a config HAProxy is already running. Without promoting it, the live + map drifts from its backup, create_backup() misses its fast path on the + NEXT config change, and an edge that blocks IPs automatically (this fleet + does) pays an extra `haproxy -c` on the following customer-facing domain + add - permanently, since every block re-opens the drift. + + Guarded twice: + * a config backup set must already exist - never fabricate a rollback + target out of nothing (see restore_backup()); and + * the map must be well-formed, so promoting it cannot leave a "rollback + target" HAProxy refuses to load. That would be this module's own bug + on a different file. + When either fails we leave the older backup map alone and the next + create_backup() takes the slow, `haproxy -c`-validated path - i.e. the + behaviour before this function existed. Nothing here can lose data. + + Note the map is promoted after the write rather than after the caller's + reload: the routes only warn on a failed reload and carry on, so there is + no reload result to gate on. A well-formed map that HAProxy has not loaded + yet is still a loadable rollback target, which is the guarantee that + matters. + """ + if not os.path.exists(HAPROXY_BACKUP_PATH): + return False + if not _blocked_ips_map_is_wellformed(BLOCKED_IPS_MAP_PATH): + logger.warning( + f"Not recording {BLOCKED_IPS_MAP_PATH} as known-good: it contains " + "an entry HAProxy cannot parse, which would make the whole " + "configuration invalid. Keeping the previous backup map." + ) + return False + try: + shutil.copy2(BLOCKED_IPS_MAP_PATH, BLOCKED_IPS_MAP_BACKUP_PATH) + return True + except OSError as e: + logger.warning(f"Could not update the backup blocked IPs map: {e}") + return False + + +def update_blocked_ips_map(promote_backup=True): + """Update the blocked IPs map file from database. + + promote_backup=False is for callers that are in the middle of an + unvalidated configuration change - generate_config() has already taken the + rollback snapshot by the time it gets here, so refreshing the backup map + would overwrite the very bytes rollback needs. + """ try: with sqlite3.connect(DB_FILE) as conn: cursor = conn.cursor() @@ -2415,9 +2508,19 @@ def update_blocked_ips_map(): # For IP blocking, we use: 1 # This allows map_ip() to work with both single IPs and CIDR ranges os.makedirs(os.path.dirname(BLOCKED_IPS_MAP_PATH), exist_ok=True) - with open(BLOCKED_IPS_MAP_PATH, 'w') as f: - for ip in blocked_ips: - f.write(f"{ip} 1\n") + # Atomically, for the same reason haproxy.cfg is: `haproxy -c` LOADS + # this file (hap_listener.tpl matches on + # map_ip(/etc/haproxy/blocked_ips.map,0)), and a half-written final + # line is a FATAL config error rather than one dropped entry - verified + # against HAProxy 2.8. A truncated map is therefore exactly the failure + # this module's backup ordering exists to prevent, on a different file. + write_config_atomically( + BLOCKED_IPS_MAP_PATH, + ''.join(f"{ip} 1\n" for ip in blocked_ips) + ) + + if promote_backup: + _promote_blocked_ips_map_to_backup() logger.info(f"Updated blocked IPs map file with {len(blocked_ips)} IPs") return True diff --git a/scripts/test-config-rollback.py b/scripts/test-config-rollback.py index ad84b97..1b50725 100755 --- a/scripts/test-config-rollback.py +++ b/scripts/test-config-rollback.py @@ -31,8 +31,10 @@ __BROKEN__, which is how the tests inject an invalid configuration. """ import os +import re import sys import shutil +import inspect import sqlite3 import logging import tempfile @@ -412,12 +414,79 @@ class TestBackupPrimitives(RollbackTestCase): self.assertEqual(self.read(hm.HAPROXY_BACKUP_PATH), good) def test_backup_set_covers_every_file_generate_config_writes(self): - pairs = dict(hm._config_backup_pairs()) - for path in (hm.HAPROXY_CONFIG_PATH, hm.BLOCKED_IPS_MAP_PATH, - hm.CORAZA_SPOE_CONFIG_PATH): - self.assertIn(path, pairs, - f'{path} is written by generate_config() but is not ' - 'part of the backed-up config set') + """Derived, not restated. + + An earlier version of this test listed the three files it expected and + checked they were in the backup set, so it could never have noticed a + FOURTH file being added. Here the set of files generate_config() writes + is observed (and, for env-gated branches this fixture cannot safely + execute, read out of the source), and anything not backed up has to be + on the documented exclusion list below. + """ + # Written by generate_config() but deliberately NOT restorable, with + # the reason. Everything else must be in the backup set: `haproxy -c` + # validates the config as a set, so a file it loads that is not + # restored alongside haproxy.cfg breaks rollback. + excluded = { + # Only ever created empty-when-missing (haproxy refuses to start + # with an ACL -f pointing at a missing file); its contents are + # owned by the /suspended API, not by generate_config(), so there + # is nothing here for a config rollback to undo. + 'suspended_domains.list', + # Generated once and then read, never rewritten with new content. + # Its value is rendered INTO haproxy.cfg, so restoring an older + # haproxy.cfg alongside the current secret file is consistent. + 'cluster-secret', + } + backed_up = {os.path.basename(p) for p, _ in hm._config_backup_pairs()} + + # 1. Observed: run a generation with every patchable optional branch on + # and see what actually changed on disk. + self.add_domain('derive.example.com', 'derive_backend') + os.environ['HAPROXY_CORAZA_SPOE_BACKEND'] = '127.0.0.1:9000' + self.addCleanup(os.environ.pop, 'HAPROXY_CORAZA_SPOE_BACKEND', None) + before = self._snapshot_etc() + hm.generate_config() + after = self._snapshot_etc() + touched = {name for name, blob in after.items() + if before.get(name) != blob} + self.assertIn('coraza-spoe.cfg', touched, + 'fixture precondition: the Coraza branch did not run') + + # 2. Read out of the source: branches this fixture must not execute + # (suspension writes a hardcoded /etc/haproxy path that no test + # global can redirect) still have to be accounted for. + touched |= { + os.path.basename(m) + for m in re.findall(r"'(/etc/haproxy/[\w.+-]+)'", + inspect.getsource(hm.generate_config)) + } + + unaccounted = touched - backed_up - excluded + self.assertEqual( + unaccounted, set(), + f'generate_config() writes {sorted(unaccounted)}, which is neither ' + 'in the backup set nor on the documented exclusion list - a ' + 'rollback would restore a mixed-vintage config set') + + def _snapshot_etc(self): + """Contents of every plain file in the fake /etc/haproxy. + + Skips the backup halves (they are the thing being maintained), the + SQLite database and its journals, and the stats socket. + """ + skip_prefixes = (os.path.basename(hm.DB_FILE), + os.path.basename(hm.HAPROXY_SOCKET_PATH)) + state = {} + for name in os.listdir(self.etc): + path = os.path.join(self.etc, name) + if not os.path.isfile(path) or name.endswith('.backup'): + continue + if name.startswith(skip_prefixes): + continue + with open(path, 'rb') as fh: + state[name] = fh.read() + return state def test_coraza_spoe_config_round_trips(self): self.generate_good_config() @@ -452,8 +521,12 @@ class TestAtomicWrite(RollbackTestCase): fh.write('old content\n') # Anything that makes f.write() blow up mid-flight stands in for a full - # disk / killed container. - with self.assertRaises(Exception): + # disk / killed container. TypeError specifically, not Exception: a + # bare assertRaises(Exception) also swallows the AttributeError raised + # when write_config_atomically does not exist at all, so this test + # passed against the pre-fix tree and would keep passing if the + # function were deleted. + with self.assertRaises(TypeError): hm.write_config_atomically(path, object()) self.assertEqual(self.read(path), 'old content\n', @@ -462,6 +535,241 @@ class TestAtomicWrite(RollbackTestCase): self.assertEqual(leftovers, [], f'temp files left behind: {leftovers}') +class TestBackupFailureGuard(RollbackTestCase): + """generate_config() refuses to write when no rollback target could be taken.""" + + def test_a_failed_backup_stops_the_config_from_being_written(self): + good = self.generate_good_config() + self.block_ip('192.0.2.10') + hm.update_blocked_ips_map() + good_map = self.read(hm.BLOCKED_IPS_MAP_PATH) + + real_create_backup = hm.create_backup + hm.create_backup = lambda *a, **kw: (False, 'error') + self.addCleanup(setattr, hm, 'create_backup', real_create_backup) + + self.add_domain('second.example.com', 'second_backend', '10.0.0.3') + with self.assertRaises(Exception) as ctx: + hm.generate_config() + + self.assertIn('Refusing to regenerate', str(ctx.exception), + 'a backup failure was not reported as a refusal') + self.assertEqual( + self.read(hm.HAPROXY_CONFIG_PATH), good, + 'a new config was written even though the snapshot failed - a bad ' + 'change could not have been rolled back') + self.assertEqual( + self.read(hm.BLOCKED_IPS_MAP_PATH), good_map, + 'the blocked IPs map was rewritten even though the snapshot failed') + + +class TestValidatorAvailability(RollbackTestCase): + """'the validator could not run' is not the same as 'the config is bad'.""" + + def _hide_the_haproxy_binary(self): + empty = os.path.join(self.tmp, 'empty-bin') + os.makedirs(empty, exist_ok=True) + os.environ['PATH'] = empty + # setUp's cleanup restores the original PATH. + + def test_a_missing_validator_is_unavailable_not_invalid(self): + good = self.generate_good_config() + self._hide_the_haproxy_binary() + + status, message = hm.validate_config_file(hm.HAPROXY_CONFIG_PATH) + self.assertEqual(status, 'unavailable', + 'a validator that could not run was reported as a ' + f'verdict on the config ({status}: {message})') + + # And the consequence create_backup() draws from it: a config it could + # not check is still snapshotted, because refusing would leave the box + # with no rollback target at all. Contrast + # test_a_broken_current_config_does_not_replace_a_good_backup, where a + # real 'invalid' verdict yields 'kept_previous'. + with open(hm.HAPROXY_CONFIG_PATH, 'w') as fh: + fh.write('hand written, unverifiable\n') + ok, status = hm.create_backup() + self.assertTrue(ok) + self.assertEqual(status, 'created', + 'an unverifiable config was treated as a rejected one') + self.assertEqual(self.read(hm.HAPROXY_BACKUP_PATH), + 'hand written, unverifiable\n') + self.assertNotEqual(self.read(hm.HAPROXY_BACKUP_PATH), good) + + +class TestFileComparison(RollbackTestCase): + """The fast path compares bytes, not sizes.""" + + def test_same_size_different_content_is_not_identical(self): + a = os.path.join(self.etc, 'a') + b = os.path.join(self.etc, 'b') + with open(a, 'w') as fh: + fh.write('aaaa\n') + with open(b, 'w') as fh: + fh.write('aaba\n') + self.assertEqual(os.path.getsize(a), os.path.getsize(b), + 'fixture: the two files must be the same size') + self.assertFalse(hm._files_identical(a, b), + 'two same-size files with different bytes compared equal') + + def test_a_same_size_drifted_config_still_hits_the_validation_gate(self): + """The case the byte-compare exists for. + + A config edited in place without changing its length (one character + swapped, a hostname replaced by another of the same width) must not be + mistaken for the known-good backup and waved through. + """ + good = self.generate_good_config() + # Sized in BYTES, not characters: the rendered config contains + # non-ASCII (em dashes in template comments), so len(str) would be + # smaller than the file and this test would pass for the wrong reason. + size = os.path.getsize(hm.HAPROXY_CONFIG_PATH) + broken = f'# {BROKEN_TOKEN}\n'.encode() + broken += b'#' * (size - len(broken) - 1) + b'\n' + with open(hm.HAPROXY_CONFIG_PATH, 'wb') as fh: + fh.write(broken) + self.assertEqual(os.path.getsize(hm.HAPROXY_CONFIG_PATH), size, + 'fixture: the drifted config must be the same size') + + ok, status = hm.create_backup() + self.assertTrue(ok) + self.assertEqual( + status, 'kept_previous', + 'a same-size broken config was accepted as unchanged and skipped ' + 'the validation gate') + self.assertEqual(self.read(hm.HAPROXY_BACKUP_PATH), good, + 'the known-good backup was overwritten') + + +class TestBlockedIpsMapWrites(RollbackTestCase): + """blocked_ips.map is loaded by `haproxy -c`, so it gets the same care. + + Verified against HAProxy 2.8: with the map referenced by + map_ip(/etc/haproxy/blocked_ips.map,0), a half-written final line makes the + WHOLE configuration invalid ("'198.51.10' is not a valid IPv4 or IPv6 + address at line 2 of file ..."), not merely a dropped entry. + """ + + def test_the_map_goes_through_the_atomic_writer(self): + seen = [] + real_write = hm.write_config_atomically + + def spy(path, content, *args, **kwargs): + seen.append(path) + return real_write(path, content, *args, **kwargs) + + hm.write_config_atomically = spy + self.addCleanup(setattr, hm, 'write_config_atomically', real_write) + + self.block_ip('192.0.2.10') + self.assertTrue(hm.update_blocked_ips_map()) + self.assertIn(hm.BLOCKED_IPS_MAP_PATH, seen, + 'the blocked IPs map was written without the atomic ' + 'writer - a truncated map is a fatal config') + + def test_a_crash_before_the_rename_leaves_the_old_map_intact(self): + self.block_ip('192.0.2.10') + hm.update_blocked_ips_map() + good_map = self.read(hm.BLOCKED_IPS_MAP_PATH) + + real_replace = os.replace + + def boom(src, dst, *args, **kwargs): + if dst == hm.BLOCKED_IPS_MAP_PATH: + raise OSError('simulated crash between write and rename') + return real_replace(src, dst, *args, **kwargs) + + os.replace = boom + self.addCleanup(setattr, os, 'replace', real_replace) + + self.block_ip('198.51.100.20') + self.assertFalse(hm.update_blocked_ips_map(), + 'a failed map write was reported as success') + os.replace = real_replace + + self.assertEqual( + self.read(hm.BLOCKED_IPS_MAP_PATH), good_map, + 'an interrupted map write clobbered the map HAProxy is running') + leftovers = [n for n in os.listdir(self.etc) if n.endswith('.tmp')] + self.assertEqual(leftovers, [], f'temp files left behind: {leftovers}') + + def test_a_malformed_map_is_not_recorded_as_known_good(self): + """The map backup must stay something HAProxy would actually load.""" + self.generate_good_config() + good_map = self.read(hm.BLOCKED_IPS_MAP_BACKUP_PATH) + + # Straight into the table, the way a bad row gets there in the first + # place - the API route is not the only writer. + self.block_ip('not-an-ip') + hm.update_blocked_ips_map() + + self.assertEqual( + self.read(hm.BLOCKED_IPS_MAP_BACKUP_PATH), good_map, + 'a map HAProxy cannot parse was promoted to the rollback target') + + def test_no_map_backup_is_fabricated_before_a_config_exists(self): + """Nothing to stay in step with means nothing to write.""" + self.block_ip('192.0.2.10') + self.assertTrue(hm.update_blocked_ips_map()) + self.assertFalse( + os.path.exists(hm.BLOCKED_IPS_MAP_BACKUP_PATH), + 'a rollback target was invented out of a map write alone') + + +class TestValidationCost(RollbackTestCase): + """`haproxy -c` runs are the customer-facing cost of a config change. + + generate_config() runs synchronously inside the API call that adds a + domain, and on an edge with hundreds of certificates `haproxy -c` is the + expensive part. These counts are the contract; changing them should be a + deliberate decision, not a side effect. + """ + + def _count_validations(self, action): + calls = [] + real_validate = hm.validate_config_file + + def spy(path): + calls.append(path) + return real_validate(path) + + hm.validate_config_file = spy + try: + action() + finally: + hm.validate_config_file = real_validate + return len(calls) + + def test_blocking_an_ip_does_not_add_a_validation_to_the_next_change(self): + self.generate_good_config() + + def add(domain, backend, address): + self.add_domain(domain, backend, address) + hm.generate_config() + + steady = self._count_validations( + lambda: add('a.example.com', 'a_backend', '10.0.0.4')) + self.assertEqual( + steady, 1, + 'a steady-state config change should cost exactly one `haproxy -c` ' + '(the pre-reload gate); the known-good fast path should skip the ' + f'other one, but {steady} ran') + + # What POST /api/blocked-ips does: rewrite the map outside + # generate_config(). This fleet blocks IPs automatically, so it happens + # between most config changes. + self.block_ip('192.0.2.10') + hm.update_blocked_ips_map() + + after_block = self._count_validations( + lambda: add('b.example.com', 'b_backend', '10.0.0.5')) + self.assertEqual( + after_block, steady, + 'an IP block left the map out of step with its backup, so the next ' + f'domain add paid {after_block} `haproxy -c` runs instead of ' + f'{steady} - on the customer-facing call') + + if __name__ == '__main__': print(f"testing haproxy_manager from: {MODULE_DIR}") unittest.main(verbosity=2) From 22dab685d03d904333fc456b53153aeecc2716b0 Mon Sep 17 00:00:00 2001 From: Josh Knapp Date: Thu, 6 Aug 2026 08:58:47 -0700 Subject: [PATCH 3/4] 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) --- haproxy_manager.py | 578 +++++++++++++++++++++-- scripts/cert-publish-lib.sh | 294 ++++++++++++ scripts/renew-certificates.sh | 33 +- scripts/sync-certificates.sh | 33 +- scripts/test-cert-scripts.py | 636 ++++++++++++++++++++++++++ scripts/test-cert-write-safety.py | 737 ++++++++++++++++++++++++++++++ 6 files changed, 2254 insertions(+), 57 deletions(-) create mode 100644 scripts/cert-publish-lib.sh create mode 100755 scripts/test-cert-scripts.py create mode 100755 scripts/test-cert-write-safety.py diff --git a/haproxy_manager.py b/haproxy_manager.py index 7390cd6..2780673 100644 --- a/haproxy_manager.py +++ b/haproxy_manager.py @@ -113,6 +113,12 @@ BLOCKED_IPS_MAP_BACKUP_PATH = '/etc/haproxy/blocked_ips.map.backup' CORAZA_SPOE_CONFIG_PATH = '/etc/haproxy/coraza-spoe.cfg' CORAZA_SPOE_BACKUP_PATH = '/etc/haproxy/coraza-spoe.cfg.backup' HAPROXY_SOCKET_PATH = '/var/run/haproxy.sock' +# HAProxy loads this path as a DIRECTORY (`bind ... ssl crt /etc/haproxy/certs`), +# which means it tries to load EVERY file it finds in here. Nothing but final, +# validated `.pem` bundles may ever exist in this directory - no temp +# files, no `.backup` copies. Staging and backups live in the sibling +# directories below, on the same filesystem so os.replace() stays atomic. +# See publish_pem_bundle(). SSL_CERTS_DIR = '/etc/haproxy/certs' # Stable per-host secret for QUIC Retry/address-validation tokens. Lives in the # /etc/haproxy named volume so it survives container recreates; self-healed on @@ -309,6 +315,298 @@ def find_certbot_live_dir(base_domain): candidates.sort(key=lambda x: x[1], reverse=True) return candidates[0][0] +# --------------------------------------------------------------------------- +# Certificate publishing +# --------------------------------------------------------------------------- +# Until 2026-08 every code path that refreshed a combined PEM did this: +# +# with open(combined_path, 'w') as combined: # TRUNCATES the file +# subprocess.run(['cat', cert, key], stdout=combined) # rc ignored +# +# `combined_path` is the live bundle HAProxy is serving. open(...,'w') empties +# it BEFORE a single byte of source material has been read, and the `cat` exit +# status was never checked. Any failure in between - source unreadable, disk +# full, container killed, certbot lineage half-written - 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 `bind ... ssl crt` and takes down HTTPS +# for every site on the host. Unlike a bad haproxy.cfg this is NOT recoverable +# by config rollback, and re-issuing hits Let's Encrypt rate limits. +# +# Everything below exists to make publishing a bundle all-or-nothing: +# assemble in a staging dir -> validate -> back up the old one -> os.replace() +# The live file is only ever swapped for a complete, validated replacement. + + +class CertificatePublishError(Exception): + """A certificate bundle could not be published. The live PEM is untouched.""" + + +def _cert_sibling_dir(name): + """A directory next to SSL_CERTS_DIR (NOT inside it). + + HAProxy loads SSL_CERTS_DIR as a crt directory and tries to load every file + in it, so staging and backup copies must live outside. Siblings share the + /etc/haproxy filesystem, which is what keeps os.replace() atomic. + + Derived at call time so tests that repoint SSL_CERTS_DIR get matching + staging/backup dirs, the same pattern as _config_backup_pairs(). + """ + parent = os.path.dirname(SSL_CERTS_DIR.rstrip('/')) or '/' + return os.path.join(parent, name) + + +def cert_staging_dir(): + """Directory where bundles are assembled and validated before publishing.""" + return _cert_sibling_dir('cert-staging') + + +def cert_backup_dir(): + """Directory holding the previous copy of each published bundle. + + Mirrors the config backup on the parent branch (one `.backup` alongside + haproxy.cfg): one copy per cert, overwritten on each successful publish, so + an operator always has a manual path back to the bundle that was being + served before the last change. + """ + return _cert_sibling_dir('cert-backups') + + +# 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. +_PEM_KEY_LABELS = ('PRIVATE KEY', 'RSA PRIVATE KEY', 'EC PRIVATE KEY') + + +def _pem_labels(text): + """Labels of well-formed PEM blocks in text, in order. + + A block counts only if its BEGIN line is followed by the matching END line, + so a bundle truncated in the middle of a block yields no label for it - + which is exactly the corruption we are guarding against. + """ + labels = [] + open_label = None + for line in text.splitlines(): + line = line.strip() + if line.startswith('-----BEGIN ') and line.endswith('-----'): + open_label = line[len('-----BEGIN '):-len('-----')].strip() + elif line.startswith('-----END ') and line.endswith('-----'): + end_label = line[len('-----END '):-len('-----')].strip() + if open_label is not None and end_label == open_label: + labels.append(end_label) + open_label = None + return labels + + +def validate_pem_structure(text): + """Structural check on an assembled bundle. Returns (ok, message). + + Pure Python and therefore ALWAYS available - it can never be skipped for + 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. + """ + if not text.strip(): + return False, 'bundle is empty' + labels = _pem_labels(text) + if not labels: + return False, 'bundle contains no complete PEM block (truncated?)' + if 'CERTIFICATE' not in labels: + return False, 'bundle contains no complete CERTIFICATE block' + if not any(label in _PEM_KEY_LABELS for label in labels): + return False, 'bundle contains no complete private key block' + return True, None + + +def _openssl_pairing_status(path): + """Does the private key in `path` match the leaf certificate in `path`? + + Returns (status, message) with status 'valid' | 'invalid' | 'unavailable'. + + `openssl x509` reads the FIRST certificate in the file (our bundles are + fullchain-then-key, so that is the leaf) and `openssl pkey` scans past the + certificate blocks to the first private key, so both run against the + 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. + """ + try: + cert_pub = subprocess.run( + ['openssl', 'x509', '-in', path, '-noout', '-pubkey'], + capture_output=True, text=True, stdin=subprocess.DEVNULL) + except FileNotFoundError: + return 'unavailable', 'openssl binary not found' + except Exception as e: + return 'unavailable', f'could not run openssl: {e}' + if cert_pub.returncode != 0: + return 'invalid', ('leaf certificate is unreadable: ' + f'{(cert_pub.stderr or "").strip()[:200]}') + try: + # -passin pass: plus a closed stdin so an (unexpected) encrypted key + # fails fast instead of blocking on a passphrase prompt. + key_pub = subprocess.run( + ['openssl', 'pkey', '-in', path, '-pubout', '-passin', 'pass:'], + capture_output=True, text=True, stdin=subprocess.DEVNULL) + except FileNotFoundError: + return 'unavailable', 'openssl binary not found' + except Exception as e: + return 'unavailable', f'could not run openssl: {e}' + if key_pub.returncode != 0: + return 'invalid', ('private key is unreadable: ' + f'{(key_pub.stderr or "").strip()[:200]}') + if cert_pub.stdout.strip() != key_pub.stdout.strip(): + return 'invalid', 'private key does not match the leaf certificate' + return 'valid', None + + +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). + """ + try: + with open(path, 'r') as fh: + text = fh.read() + except OSError as e: + return False, f'cannot read assembled bundle: {e}' + + ok, msg = validate_pem_structure(text) + if not ok: + return False, msg + + status, pair_msg = _openssl_pairing_status(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) + return True, None + + +def backup_existing_pem(dest_path): + """Copy the currently published bundle aside before it is replaced. + + Only a bundle that still validates is promoted to backup: overwriting a + good backup with an already-corrupt live file would turn "restore the + backup" into "restore different garbage". Same require_valid reasoning as + create_backup() for haproxy.cfg. + + Never fatal - failing to take a backup must not stop us replacing a cert + with a validated one - but always logged. + """ + if not os.path.exists(dest_path): + return None + try: + with open(dest_path, 'r') as fh: + ok, msg = validate_pem_structure(fh.read()) + except OSError as e: + ok, msg = False, str(e) + backup_path = os.path.join(cert_backup_dir(), os.path.basename(dest_path)) + if not ok: + logger.warning( + "Not backing up %s before replacing it: the file on disk is not a " + "valid bundle (%s). Keeping any previous backup at %s.", + dest_path, msg, backup_path) + return None + try: + os.makedirs(cert_backup_dir(), exist_ok=True) + shutil.copy2(dest_path, backup_path) + return backup_path + except Exception as e: + logger.error("Failed to back up %s to %s: %s", + dest_path, backup_path, e) + return None + + +def publish_pem_bundle(dest_path, source_paths): + """Publish cert+key as a combined PEM at dest_path, atomically. + + source_paths are concatenated in order (fullchain first, then privkey) into + a staging file OUTSIDE the crt directory, validated there, and only then + moved into place with os.replace(). If anything fails, the exception is + raised and dest_path still holds the previous, working bundle - the live + PEM is never opened for writing at any point. + + Returns the path of the backup taken (or None). Raises + CertificatePublishError on any failure. + """ + for src in source_paths: + if not os.path.exists(src): + raise CertificatePublishError( + f'source certificate material missing: {src}') + + parts = [] + for src in source_paths: + try: + with open(src, 'r') as fh: + data = fh.read() + except OSError as e: + raise CertificatePublishError(f'cannot read {src}: {e}') + if not data.strip(): + raise CertificatePublishError(f'source file is empty: {src}') + if not data.endswith('\n'): + # Guard against a cert whose last line runs into the key's BEGIN + # line; certbot always ends with a newline, but a hand-placed file + # might not. + data += '\n' + parts.append(data) + content = ''.join(parts) + + ok, msg = validate_pem_structure(content) + if not ok: + raise CertificatePublishError( + f'assembled bundle for {dest_path} is not usable: {msg}') + + dest_dir = os.path.dirname(dest_path) or '.' + try: + os.makedirs(dest_dir, exist_ok=True) + os.makedirs(cert_staging_dir(), exist_ok=True) + except OSError as e: + raise CertificatePublishError(f'cannot prepare directories: {e}') + + # os.replace() is only atomic within one filesystem. Checking up front + # turns an EXDEV rename failure into an operator-readable message; the + # outcome is the same either way (nothing published, live PEM untouched) + # because staging inside the crt directory is not an acceptable fallback. + try: + if os.stat(cert_staging_dir()).st_dev != os.stat(dest_dir).st_dev: + raise CertificatePublishError( + f'{cert_staging_dir()} and {dest_dir} are on different ' + 'filesystems, so a certificate cannot be swapped in atomically') + except OSError as e: + raise CertificatePublishError(f'cannot stat certificate dirs: {e}') + + backup_path = backup_existing_pem(dest_path) + + def _validate_staged(staged_path): + return validate_pem_bundle(staged_path) + + try: + write_config_atomically(dest_path, content, + staging_dir=cert_staging_dir(), + validate=_validate_staged) + except Exception as e: + raise CertificatePublishError( + f'refused to publish {dest_path}: {e}. The previously served ' + 'certificate is still in place.') + logger.info("Published validated certificate bundle to %s%s", dest_path, + f' (previous copy backed up to {backup_path})' + if backup_path else '') + return backup_path + + def certbot_register(): """Register with Let's Encrypt using the certbot client and agree to the terms of service""" result = subprocess.run(['certbot', 'show_account'], capture_output=True) @@ -337,12 +635,26 @@ def generate_self_signed_cert(ssl_certs_dir): '-subj', f'/CN={DOMAIN}' ], check=True) - # Combine cert and key for HAProxy - with open(self_sign_cert, 'wb') as combined: + # Combine cert and key for HAProxy. Same publisher as every other bundle: + # this file lands in the crt directory, so a half-written one would break + # the TLS bind for every site on the host, and because the function + # short-circuits on "file exists" a corrupt one would never be regenerated. + try: + publish_pem_bundle(self_sign_cert, ['/tmp/cert.pem', '/tmp/key.pem']) + except CertificatePublishError as e: + # do_initial_setup() calls this unguarded, so raising here would abort + # container startup before HAProxy is ever launched. Refusing to write + # an unusable default cert is right; taking the whole container down + # over it is not - start_haproxy() already degrades gracefully. + logger.critical("Could not publish the default self-signed certificate " + "to %s: %s", self_sign_cert, e) + return False + finally: for file in ['/tmp/cert.pem', '/tmp/key.pem']: - with open(file, 'rb') as f: - combined.write(f.read()) - os.remove(file) # Clean up temporary files + try: + os.remove(file) # Clean up temporary files + except OSError: + pass generate_config() return True @@ -581,8 +893,15 @@ def request_ssl(): # Ensure SSL certs directory exists os.makedirs(SSL_CERTS_DIR, exist_ok=True) - with open(combined_path, 'w') as combined: - subprocess.run(['cat', cert_path, key_path], stdout=combined) + try: + publish_pem_bundle(combined_path, [cert_path, key_path]) + except CertificatePublishError as e: + # Nothing was written: any previously served bundle for this + # name is untouched and HAProxy is not reloaded. + error_msg = f'Certificate issued but not published: {e}' + logger.critical(error_msg) + log_operation('request_ssl', False, error_msg) + return jsonify({'status': 'error', 'message': error_msg}), 500 # Update database with sqlite3.connect(DB_FILE) as conn: @@ -611,8 +930,8 @@ def request_ssl(): log_operation('request_ssl', False, str(e)) return jsonify({'status': 'error', 'message': str(e)}), 500 -def _cleanup_superseded_lineages(keep_path, keep_lineage, bundle_names): - """Remove cert files + certbot lineages that the just-issued bundle supersedes. +def _quarantine_superseded_certs(keep_path, keep_lineage, bundle_names): + """Move aside cert files that the just-issued bundle supersedes. A `.pem` in /etc/haproxy/certs/ is "superseded" iff its certificate's CN is one of the bundle's names AND the file isn't the bundle's own combined @@ -620,10 +939,24 @@ def _cleanup_superseded_lineages(keep_path, keep_lineage, bundle_names): since that's what HAProxy SNI-matches against and what the file convention names it after. - Also drops the corresponding certbot renewal config so `certbot renew` - stops trying to renew the dead lineage on its next 12h cron tick. + This used to os.remove() the file and immediately `certbot delete` the + lineage, both BEFORE anything had checked that the replacement bundle was + usable — destroying the only two copies of a working certificate on the + strength of a `cat` whose exit status nobody read. Now: - Returns a small summary dict for logging / API response. + * the superseded file is MOVED to the cert backup directory instead of + unlinked, so an operator can put it back by hand; + * the certbot lineage is left alone here. Deleting it is irreversible + (archive, live and renewal config all go) and recovering means fresh, + rate-limited ACME orders, so it happens only in + _delete_superseded_lineages(), after HAProxy has actually loaded the + replacement. + + Moving still solves the problem the removal existed for: the old file no + longer sits in the crt directory shadowing the new bundle's SNI match. + + Returns a summary dict for logging / API response. Entries in 'removed' + carry the lineage name for the deletion phase. """ summary = {'removed': [], 'errors': [], 'skipped': []} @@ -675,26 +1008,54 @@ def _cleanup_superseded_lineages(keep_path, keep_lineage, bundle_names): continue try: - os.remove(fpath) - removed_entry = {'file': fname, 'cn': cn, 'lineage_deleted': False} - # Best-effort certbot lineage delete. Some files may not have a - # corresponding lineage (e.g. self-signed dev certs); ignore those. - try: - cb_proc = subprocess.run( - ['certbot', 'delete', '--cert-name', lineage_name, '-n'], - capture_output=True, text=True - ) - removed_entry['lineage_deleted'] = (cb_proc.returncode == 0) - if cb_proc.returncode != 0: - removed_entry['certbot_stderr'] = (cb_proc.stderr or '').strip()[:200] - except Exception as e: - removed_entry['certbot_error'] = str(e) - summary['removed'].append(removed_entry) + os.makedirs(cert_backup_dir(), exist_ok=True) + quarantine_path = os.path.join(cert_backup_dir(), fname) + # Move, not unlink: out of the crt directory (so it stops shadowing + # the new bundle) but still on disk for manual recovery. + shutil.move(fpath, quarantine_path) + summary['removed'].append({ + 'file': fname, + 'cn': cn, + 'lineage': lineage_name, + 'moved_to': quarantine_path, + 'lineage_deleted': False, + }) except Exception as e: summary['errors'].append({'file': fname, 'error': str(e)}) return summary + +def _delete_superseded_lineages(summary): + """`certbot delete` the lineages quarantined by _quarantine_superseded_certs(). + + IRREVERSIBLE: certbot removes the lineage's archive, live symlinks and + renewal config. If it turns out we needed that certificate, the only way + back is a fresh ACME order, which Let's Encrypt rate-limits — an outage + measured in hours. So this is gated hardest of anything in this module: it + runs only after the replacement bundle has been assembled, validated, + published, AND loaded by a HAProxy that reloaded successfully. + + Mutates `summary` in place. Best-effort: some files have no corresponding + lineage (e.g. self-signed dev certs) and a failure here is harmless — a + dead lineage merely wastes a renewal attempt on the next 12h cron tick. + """ + for entry in summary.get('removed', []): + lineage_name = entry.get('lineage') + if not lineage_name: + continue + try: + cb_proc = subprocess.run( + ['certbot', 'delete', '--cert-name', lineage_name, '-n'], + capture_output=True, text=True + ) + entry['lineage_deleted'] = (cb_proc.returncode == 0) + if cb_proc.returncode != 0: + entry['certbot_stderr'] = (cb_proc.stderr or '').strip()[:200] + except Exception as e: + entry['certbot_error'] = str(e) + return summary + @app.route('/api/ssl/bundle', methods=['POST']) @require_api_key def request_ssl_bundle(): @@ -794,8 +1155,17 @@ def request_ssl_bundle(): combined_path = f'{SSL_CERTS_DIR}/{primary}.pem' os.makedirs(SSL_CERTS_DIR, exist_ok=True) - with open(combined_path, 'w') as combined: - subprocess.run(['cat', cert_path, key_path], stdout=combined) + try: + publish_pem_bundle(combined_path, [cert_path, key_path]) + except CertificatePublishError as e: + # Publishing is all-or-nothing, so at this point nothing has been + # written, no old certificate has been touched and no lineage has + # been deleted. Stop before any of that becomes untrue. + error_msg = f'Bundle issued but not published for {primary}: {e}' + logger.critical(error_msg) + log_operation('request_ssl_bundle', False, error_msg) + return jsonify({'status': 'error', 'message': error_msg, + 'primary': primary, 'names': names}), 500 # Mark every name in the bundle as ssl_enabled, all pointing at the # same combined .pem. HAProxy serves one file for many SNI hostnames. @@ -816,15 +1186,35 @@ def request_ssl_bundle(): # `bind ... ssl crt /etc/haproxy/certs` directive. HAProxy then picks # one of them by alphabetical/load order — frequently the older # single-SAN file — and the new bundle has no effect on what's served. - # This block deletes those superseded files (and their certbot lineage) + # This block moves those superseded files out of the crt directory # before the generate_config() reload so HAProxy picks up the bundle. - cleanup_summary = _cleanup_superseded_lineages( + # It only runs once publish_pem_bundle() above has validated the + # replacement and put it in place, so the old certificate is never the + # only copy we have. + cleanup_summary = _quarantine_superseded_certs( keep_path=combined_path, keep_lineage=primary, bundle_names=set(names), ) - generate_config() + # Raises if the config does not validate or HAProxy does not reload; + # the certbot lineages below are therefore only deleted once the new + # bundle is genuinely being served. + try: + generate_config() + except Exception: + if cleanup_summary['removed']: + logger.critical( + "HAProxy did not reload after publishing the bundle for " + "%s. The superseded certificate files were moved to %s and " + "their certbot lineages were NOT deleted, so they can be " + "restored by hand: %s", + primary, cert_backup_dir(), + [e['file'] for e in cleanup_summary['removed']]) + raise + + _delete_superseded_lineages(cleanup_summary) + log_operation( 'request_ssl_bundle', True, f'SSL bundle issued for {primary} covering {len(names)} names; ' @@ -861,11 +1251,12 @@ def renew_certificates(): # Check if any certificates were renewed if 'Congratulations' in result.stdout or 'renewed' in result.stdout: # Update combined certificates for HAProxy + publish_failures = [] with sqlite3.connect(DB_FILE) as conn: cursor = conn.cursor() cursor.execute('SELECT domain, ssl_cert_path FROM domains WHERE ssl_enabled = 1') domains = cursor.fetchall() - + for domain, cert_path in domains: if cert_path and os.path.exists(cert_path): # For wildcard domains, strip *. prefix for directory lookup @@ -876,15 +1267,40 @@ def renew_certificates(): letsencrypt_key = os.path.join(live_dir, 'privkey.pem') if os.path.exists(letsencrypt_cert) and os.path.exists(letsencrypt_key): - with open(cert_path, 'w') as combined: - subprocess.run(['cat', letsencrypt_cert, letsencrypt_key], stdout=combined) - - # Regenerate config and reload HAProxy + # A failure here leaves the currently + # served bundle in place. That certificate + # is still valid (renewal runs ~30 days + # before expiry and retries every 12h), so + # keeping it is strictly better than + # replacing it with something unverified. + try: + publish_pem_bundle( + cert_path, + [letsencrypt_cert, letsencrypt_key]) + except CertificatePublishError as e: + logger.critical( + "Renewed certificate for %s was NOT " + "published: %s", domain, e) + publish_failures.append( + {'domain': domain, 'error': str(e)}) + + # Regenerate config and reload HAProxy. Safe to do with + # publish failures present: those certificates were left + # untouched, so nothing unvalidated is being loaded. generate_config() reload_result = subprocess.run('echo "reload" | socat stdio /tmp/haproxy-cli', capture_output=True, text=True, shell=True) if reload_result.returncode == 0: + if publish_failures: + error_msg = ( + f'{len(publish_failures)} renewed certificate(s) could ' + 'not be published and are still being served from ' + 'their previous bundle') + log_operation('renew_certificates', False, error_msg) + return jsonify({'status': 'partial_success', + 'message': error_msg, + 'failures': publish_failures}), 500 log_operation('renew_certificates', True, 'Certificates renewed and HAProxy reloaded') return jsonify({'status': 'success', 'message': 'Certificates renewed and HAProxy reloaded'}) else: @@ -1077,9 +1493,19 @@ def request_certificates(): # Ensure SSL certs directory exists os.makedirs(SSL_CERTS_DIR, exist_ok=True) - with open(combined_path, 'w') as combined: - subprocess.run(['cat', cert_path, key_path], stdout=combined) - + try: + publish_pem_bundle(combined_path, [cert_path, key_path]) + except CertificatePublishError as e: + error_msg = f'Certificate issued but not published: {e}' + logger.critical('%s (%s)', error_msg, domain) + results.append({ + 'domain': domain, + 'status': 'error', + 'message': error_msg, + }) + error_count += 1 + continue + # Update database (add domain if it doesn't exist) with sqlite3.connect(DB_FILE) as conn: cursor = conn.cursor() @@ -1700,11 +2126,13 @@ def dns_challenge_verify(): os.makedirs(SSL_CERTS_DIR, exist_ok=True) combined_path = f'{SSL_CERTS_DIR}/_wildcard_.{base_domain}.pem' - with open(combined_path, 'w') as combined: - with open(cert_path, 'r') as cf: - combined.write(cf.read()) - with open(key_path, 'r') as kf: - combined.write(kf.read()) + try: + publish_pem_bundle(combined_path, [cert_path, key_path]) + except CertificatePublishError as e: + error_msg = f'Wildcard certificate obtained but not published: {e}' + logger.critical(error_msg) + log_operation('dns_challenge_verify', False, error_msg) + return jsonify({'success': False, 'error': error_msg}), 500 # Update database with sqlite3.connect(DB_FILE) as conn: @@ -1750,6 +2178,40 @@ def get_or_create_cluster_secret(): secret = f.read().strip() if secret: return secret + # File exists but is blank — e.g. a create that died between + # open() and write(), or a volume restored empty. Without healing + # it here we fall through to the O_EXCL create below, which fails + # with FileExistsError, and the handler re-reads the same blank + # file: this function would return '' forever and the host would + # never get the stable secret its docstring promises. + # + # Rewrite in place under an exclusive lock rather than unlinking + # and recreating: the file is never momentarily absent, and two + # workers healing at once serialise instead of racing to install + # two different secrets. + try: + fd = os.open(CLUSTER_SECRET_PATH, os.O_RDWR) + try: + fcntl.flock(fd, fcntl.LOCK_EX) + # Re-read under the lock: another worker may have healed it + # while we were waiting. + existing = os.read(fd, 4096).decode(errors='replace').strip() + if existing: + return existing + secret = os.urandom(32).hex() + os.ftruncate(fd, 0) + os.lseek(fd, 0, os.SEEK_SET) + os.write(fd, secret.encode()) + os.fchmod(fd, 0o600) + logger.warning( + "Healed empty QUIC cluster-secret at %s", + CLUSTER_SECRET_PATH) + return secret + finally: + os.close(fd) + except Exception as e: + logger.error("Failed to heal empty cluster-secret: %s", e) + return '' # Generate and persist exclusively (0600). hex => config-safe charset. secret = os.urandom(32).hex() fd = os.open(CLUSTER_SECRET_PATH, os.O_CREAT | os.O_EXCL | os.O_WRONLY, 0o600) @@ -2111,7 +2573,7 @@ def _config_backup_pairs(): ) -def write_config_atomically(path, content): +def write_config_atomically(path, content, staging_dir=None, validate=None): """Write content to path via temp file + rename. A half-written haproxy.cfg (disk full, container killed mid-write) is just @@ -2119,8 +2581,26 @@ def write_config_atomically(path, content): atomic within a filesystem, so the file on disk is always either the whole old config or the whole new one — never a truncated hybrid. This also keeps the "existing config is already broken" case from being self-inflicted. + + The same guarantee is what certificate bundles need, so this is the single + atomic publisher for both (see publish_pem_bundle()); the two extra + arguments exist for that caller: + + staging_dir: where the temp file is created. Defaults to the destination's + own directory, which is right for /etc/haproxy but WRONG for + /etc/haproxy/certs — HAProxy loads that path as a crt directory and + tries to load every file in it, so a `.tmp` there (or one leaked by a + crash) can break the whole TLS bind. Must be on the same filesystem as + `path` or os.replace() cannot be atomic; if it is not, the rename fails + loudly and the destination is left untouched, which is the safe outcome. + + validate: optional callable(temp_path) -> (ok, message), run on the staged + file BEFORE it is moved into place. Returning False aborts the publish + with the temp file removed and `path` still holding its previous + contents. This is the only ordering that lets us validate the + replacement without having destroyed the original first. """ - directory = os.path.dirname(path) or '.' + directory = staging_dir or os.path.dirname(path) or '.' # Preserve the mode of the file we are replacing; mkstemp defaults to 0600 # and HAProxy config files are conventionally 0644. try: @@ -2136,6 +2616,10 @@ def write_config_atomically(path, content): f.flush() os.fsync(f.fileno()) os.chmod(tmp_path, mode) + if validate is not None: + ok, message = validate(tmp_path) + if not ok: + raise ValueError(f'staged file failed validation: {message}') os.replace(tmp_path, path) except Exception: try: diff --git a/scripts/cert-publish-lib.sh b/scripts/cert-publish-lib.sh new file mode 100644 index 0000000..58a382d --- /dev/null +++ b/scripts/cert-publish-lib.sh @@ -0,0 +1,294 @@ +# shellcheck shell=bash +# cert-publish-lib.sh - safe publication of HAProxy certificate bundles. +# +# This file is SOURCED, never executed (hence no shebang / no exec bit). +# +# Why this exists +# --------------- +# renew-certificates.sh and sync-certificates.sh used to publish a bundle with +# +# cat "$CERT_FILE" "$KEY_FILE" > "$COMBINED_FILE" +# +# where $COMBINED_FILE is the LIVE pem HAProxy is serving right now. The shell +# truncates the destination to zero bytes when it sets up the redirect, BEFORE +# cat ever runs, so any failure after that point (unreadable source, ENOSPC, +# container killed mid-write) leaves a zero-length or key-less pem in place. +# Checking cat's exit status does not help: the damage is already done. +# +# That matters more here than for an ordinary file because HAProxy loads +# $SSL_CERTS_DIR as a DIRECTORY: +# +# bind 0.0.0.0:443 ssl crt /etc/haproxy/certs +# +# It tries to load *every* file in that directory, and one unloadable file +# fails the whole bind - i.e. HTTPS goes down for every customer on the host. +# +# Two consequences drive the design below: +# 1. Assemble somewhere else and rename into place, so the live pem is either +# the old bundle or the new one and never a half-written one. +# 2. NEVER create a temp file, .tmp, .backup or any other non-final file +# inside $SSL_CERTS_DIR. Staging and backups live in SIBLING directories. +# +# Directory layout (kept identical to the Python half in haproxy_manager.py): +# 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. + +# 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). +declare -F log_info >/dev/null || log_info() { + echo "[$(date '+%Y-%m-%d %H:%M:%S')] [INFO] $*" +} +declare -F log_error >/dev/null || log_error() { + echo "[$(date '+%Y-%m-%d %H:%M:%S')] [ERROR] $*" >&2 +} +# log_warn is not part of the callers' vocabulary; route it through log_info +# with a loud prefix so it lands in the main log without tripping the +# error-log monitors (scripts/monitor-errors.sh) for non-fatal conditions. +declare -F log_warn >/dev/null || log_warn() { + log_info "WARNING: $*" +} + +cert_staging_dir() { + if [ -n "${CERT_STAGING_DIR:-}" ]; then + echo "$CERT_STAGING_DIR" + else + echo "$(dirname "${SSL_CERTS_DIR:-/etc/haproxy/certs}")/cert-staging" + fi +} + +cert_backup_dir() { + if [ -n "${CERT_BACKUP_DIR:-}" ]; then + echo "$CERT_BACKUP_DIR" + else + echo "$(dirname "${SSL_CERTS_DIR:-/etc/haproxy/certs}")/cert-backups" + fi +} + +# cert_bundle_valid FILE +# +# 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. +# +# 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. +cert_bundle_valid() { + local file="$1" + + if [ -z "$file" ]; then + log_error "cert_bundle_valid: no file given" + return 1 + fi + if [ ! -f "$file" ]; then + log_error "Certificate bundle $file does not exist (or is not a regular file)" + return 1 + fi + if [ ! -s "$file" ]; then + log_error "Certificate bundle $file is empty" + return 1 + fi + + # --- layer 1: structure ------------------------------------------------- + if ! grep -qF -- '-----BEGIN CERTIFICATE-----' "$file"; then + log_error "Certificate bundle $file contains no certificate block" + return 1 + fi + if ! grep -qF -- '-----END CERTIFICATE-----' "$file"; 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")" + 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 + 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 + + local cert_pub key_pub + # /dev/null /dev/null /dev/null + + base="$(basename "$dest_file")" + tmp="$(mktemp "${staging_dir}/${base}.XXXXXX" 2>/dev/null)" + if [ -z "$tmp" ] || [ ! -f "$tmp" ]; then + log_error "cert_publish: cannot create a staging file in $staging_dir" + return 1 + fi + # 0600 while the temp file holds a private key; the final mode is matched + # to the file being replaced just before the swap (see below). + chmod 600 "$tmp" 2>/dev/null + + if ! cat "$cert_file" "$key_file" > "$tmp"; then + log_error "cert_publish: failed to assemble $cert_file + $key_file (live $dest_file left untouched)" + rm -f "$tmp" + return 1 + fi + if [ ! -s "$tmp" ]; then + log_error "cert_publish: assembled bundle for $dest_file is empty (live file left untouched)" + rm -f "$tmp" + return 1 + fi + + # (c) 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 + # 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. + if [ -e "$dest_file" ]; then + backup_dir="$(cert_backup_dir)" + if cert_bundle_valid "$dest_file"; then + if mkdir -p "$backup_dir"; then + if ! cp -p "$dest_file" "${backup_dir}/${base}"; then + log_warn "could not back up $dest_file to ${backup_dir}/${base}; publishing anyway" + fi + else + log_warn "could not create backup directory $backup_dir; publishing without a backup" + fi + else + log_warn "existing $dest_file is not a valid bundle - KEEPING the previous backup" \ + "in $backup_dir rather than overwriting it with an unusable one" + fi + fi + + # Preserve the mode of the bundle being replaced (0644 by default, which is + # what `cat > file` produced under the standard umask). mktemp gives 0600, + # and mv carries the temp file's mode onto the destination, so without this + # every publish would silently tighten the live pem's permissions. Changing + # 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. + local mode + mode="$(stat -c '%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, + # 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" \ + "(live file left untouched; NOT falling back to a direct write)" + rm -f "$tmp" + return 1 + fi + + return 0 +} + +# haproxy_config_ok +# +# Gate a reload on `haproxy -c`. Returns 0 if the config validates, or if we +# cannot check (no haproxy binary) - a missing checker must not block a reload +# that is otherwise needed, but a checker that says "no" always wins. +haproxy_config_ok() { + local cfg="${HAPROXY_CONFIG:-/etc/haproxy/haproxy.cfg}" + local out rc + + if ! command -v haproxy >/dev/null 2>&1; then + log_warn "haproxy binary not found - skipping 'haproxy -c' validation before reload" + return 0 + fi + + out="$(haproxy -c -f "$cfg" 2>&1 > "$ERROR_LOG_FILE" } +# Safe certificate publication helpers (cert_publish / cert_bundle_valid / +# haproxy_config_ok). Sourced AFTER the log_* functions above so the library +# uses this script's logging rather than its own fallbacks. +SCRIPT_DIR="$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)" +# shellcheck source=cert-publish-lib.sh +if [ -r "${SCRIPT_DIR}/cert-publish-lib.sh" ]; then + . "${SCRIPT_DIR}/cert-publish-lib.sh" +else + log_error "Missing ${SCRIPT_DIR}/cert-publish-lib.sh - refusing to touch live certificates" + exit 1 +fi + log_info "Starting certificate renewal process" # Run certbot renewal — don't exit on failure, some certs may have @@ -42,7 +55,7 @@ fi mkdir -p "$SSL_CERTS_DIR" # Get all SSL-enabled domains from database -DOMAINS=$(find /etc/letsencrypt/live/ -mindepth 1 -maxdepth 1 -type d -printf '%f\n') +DOMAINS=$(find "$LETSENCRYPT_LIVE_DIR/" -mindepth 1 -maxdepth 1 -type d -printf '%f\n') if [ -z "$DOMAINS" ]; then log_info "No SSL-enabled domains found" @@ -54,13 +67,16 @@ UPDATED=0 FAILED=0 while read -r domain; do - CERT_FILE="/etc/letsencrypt/live/${domain}/fullchain.pem" - KEY_FILE="/etc/letsencrypt/live/${domain}/privkey.pem" + CERT_FILE="${LETSENCRYPT_LIVE_DIR}/${domain}/fullchain.pem" + KEY_FILE="${LETSENCRYPT_LIVE_DIR}/${domain}/privkey.pem" COMBINED_FILE="${SSL_CERTS_DIR}/${domain}.pem" if [ -f "$CERT_FILE" ] && [ -f "$KEY_FILE" ]; then - # Combine cert and key into single file for HAProxy - if cat "$CERT_FILE" "$KEY_FILE" > "$COMBINED_FILE"; then + # Assemble in a staging dir and rename into place. NEVER redirect into + # $COMBINED_FILE: the shell truncates the live pem before cat runs, and + # HAProxy loads $SSL_CERTS_DIR as a directory, so one bad file there + # takes down the whole ssl bind. See scripts/cert-publish-lib.sh. + if cert_publish "$CERT_FILE" "$KEY_FILE" "$COMBINED_FILE"; then log_info "Updated certificate for $domain" UPDATED=$((UPDATED + 1)) else @@ -77,6 +93,13 @@ log_info "Certificate update completed: $UPDATED updated, $FAILED failed" # Reload HAProxy if any certificates were updated if [ $UPDATED -gt 0 ]; then + # Never reload onto unvalidated material: a reload that fails to load the + # certs directory drops HTTPS for every site on this host. + if ! haproxy_config_ok; then + log_error "HAProxy configuration does not validate - refusing to reload after certificate renewal" + exit 1 + fi + if echo "reload" | socat stdio /tmp/haproxy-cli 2>/dev/null; then log_info "HAProxy reloaded successfully" else diff --git a/scripts/sync-certificates.sh b/scripts/sync-certificates.sh index 4743cb0..e84dd1f 100755 --- a/scripts/sync-certificates.sh +++ b/scripts/sync-certificates.sh @@ -8,6 +8,7 @@ LOG_FILE="${LOG_FILE:-/var/log/haproxy-manager.log}" ERROR_LOG_FILE="${ERROR_LOG_FILE:-/var/log/haproxy-manager-errors.log}" DB_FILE="${DB_FILE:-/etc/haproxy/haproxy_config.db}" SSL_CERTS_DIR="${SSL_CERTS_DIR:-/etc/haproxy/certs}" +LETSENCRYPT_LIVE_DIR="${LETSENCRYPT_LIVE_DIR:-/etc/letsencrypt/live}" # Logging functions log_info() { @@ -18,13 +19,25 @@ log_error() { echo "[$(date '+%Y-%m-%d %H:%M:%S')] [ERROR] $*" | tee -a "$LOG_FILE" >> "$ERROR_LOG_FILE" } +# Safe certificate publication helpers (cert_publish / cert_bundle_valid / +# haproxy_config_ok). Sourced AFTER the log_* functions above so the library +# uses this script's logging rather than its own fallbacks. +SCRIPT_DIR="$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)" +# shellcheck source=cert-publish-lib.sh +if [ -r "${SCRIPT_DIR}/cert-publish-lib.sh" ]; then + . "${SCRIPT_DIR}/cert-publish-lib.sh" +else + log_error "Missing ${SCRIPT_DIR}/cert-publish-lib.sh - refusing to touch live certificates" + exit 1 +fi + log_info "Starting certificate sync process" # Ensure SSL certs directory exists mkdir -p "$SSL_CERTS_DIR" # Get all SSL-enabled domains from database -DOMAINS=$(find /etc/letsencrypt/live/ -mindepth 1 -maxdepth 1 -type d -printf '%f\n') +DOMAINS=$(find "$LETSENCRYPT_LIVE_DIR/" -mindepth 1 -maxdepth 1 -type d -printf '%f\n') if [ -z "$DOMAINS" ]; then log_info "No SSL-enabled domains found" @@ -36,13 +49,16 @@ UPDATED=0 FAILED=0 while read -r domain; do - CERT_FILE="/etc/letsencrypt/live/${domain}/fullchain.pem" - KEY_FILE="/etc/letsencrypt/live/${domain}/privkey.pem" + CERT_FILE="${LETSENCRYPT_LIVE_DIR}/${domain}/fullchain.pem" + KEY_FILE="${LETSENCRYPT_LIVE_DIR}/${domain}/privkey.pem" COMBINED_FILE="${SSL_CERTS_DIR}/${domain}.pem" if [ -f "$CERT_FILE" ] && [ -f "$KEY_FILE" ]; then - # Combine cert and key into single file for HAProxy - if cat "$CERT_FILE" "$KEY_FILE" > "$COMBINED_FILE"; then + # Assemble in a staging dir and rename into place. NEVER redirect into + # $COMBINED_FILE: the shell truncates the live pem before cat runs, and + # HAProxy loads $SSL_CERTS_DIR as a directory, so one bad file there + # takes down the whole ssl bind. See scripts/cert-publish-lib.sh. + if cert_publish "$CERT_FILE" "$KEY_FILE" "$COMBINED_FILE"; then log_info "Updated certificate for $domain" UPDATED=$((UPDATED + 1)) else @@ -59,6 +75,13 @@ log_info "Certificate sync completed: $UPDATED updated, $FAILED failed" # Reload HAProxy if any certificates were updated if [ $UPDATED -gt 0 ]; then + # Never reload onto unvalidated material: a reload that fails to load the + # certs directory drops HTTPS for every site on this host. + if ! haproxy_config_ok; then + log_error "HAProxy configuration does not validate - refusing to reload after certificate sync" + exit 1 + fi + if echo "reload" | socat stdio /tmp/haproxy-cli 2>/dev/null; then log_info "HAProxy reloaded successfully" else diff --git a/scripts/test-cert-scripts.py b/scripts/test-cert-scripts.py new file mode 100755 index 0000000..3133280 --- /dev/null +++ b/scripts/test-cert-scripts.py @@ -0,0 +1,636 @@ +#!/usr/bin/env python3 +"""Regression tests for the certificate publication shell scripts. + +Why this file exists +-------------------- +renew-certificates.sh and sync-certificates.sh published a bundle with + + cat "$CERT_FILE" "$KEY_FILE" > "$COMBINED_FILE" + +where $COMBINED_FILE is the pem HAProxy is serving *right now*. The shell +truncates the destination when it opens the redirect, before cat runs, so any +failure after that point - unreadable source key, ENOSPC, container killed +mid-write - left a zero-length or key-less pem behind. The exit status of cat +was checked, but by then the live file was already destroyed. + +HAProxy loads $SSL_CERTS_DIR as a DIRECTORY (`bind :443 ssl crt /etc/haproxy/ +certs`) and tries to load every file in it, so a single unloadable file fails +the whole bind: HTTPS down for every customer on the host. + +The fix (scripts/cert-publish-lib.sh) assembles into a sibling staging dir, +validates, backs up the outgoing bundle into a sibling backup dir, and renames +into place. These tests pin the observable guarantees: + + * a successful publish replaces the live pem and archives the old one; + * a FAILED publish leaves the previous, still-valid pem byte-for-byte intact; + * nothing that is not a final *.pem ever appears in the certs directory; + * HAProxy is not reloaded when `haproxy -c` rejects the configuration. + +Running +------- + python3 scripts/test-cert-scripts.py # tests the repo checkout + HAPROXY_MANAGER_DIR=/some/other/tree \ + python3 scripts/test-cert-scripts.py # tests another tree + +Self-contained stdlib unittest - no pytest, no venv, no bats, and nothing is +imported from the application. The scripts are driven as subprocesses with +every path they touch redirected by environment variable (SSL_CERTS_DIR, +LETSENCRYPT_LIVE_DIR, CERT_STAGING_DIR, CERT_BACKUP_DIR, LOG_FILE, +ERROR_LOG_FILE, HAPROXY_CONFIG) and stub `certbot`, `socat` and `haproxy` +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. +""" + +import os +import re +import shutil +import subprocess +import tempfile +import textwrap +import unittest + +MODULE_DIR = os.path.abspath( + os.environ.get('HAPROXY_MANAGER_DIR', + os.path.join(os.path.dirname(os.path.abspath(__file__)), '..')) +) +SCRIPTS_DIR = os.path.join(MODULE_DIR, 'scripts') +LIB = os.path.join(SCRIPTS_DIR, 'cert-publish-lib.sh') + +BROKEN_TOKEN = '__BROKEN__' +DOMAIN = 'test.example.com' + +# --- test key material ------------------------------------------------------- +# openssl req -x509 -newkey rsa:2048 -keyout key1 -out cert1 -days 3650 -nodes \ +# -subj /CN=test.example.com +TEST_CERT = """\ +-----BEGIN CERTIFICATE----- +MIIDFzCCAf+gAwIBAgIUeaz/lNOESOTHsB3Y97+Xja7Fy4gwDQYJKoZIhvcNAQEL +BQAwGzEZMBcGA1UEAwwQdGVzdC5leGFtcGxlLmNvbTAeFw0yNjA4MDYxNTQyMTda +Fw0zNjA4MDMxNTQyMTdaMBsxGTAXBgNVBAMMEHRlc3QuZXhhbXBsZS5jb20wggEi +MA0GCSqGSIb3DQEBAQUAA4IBDwAwggEKAoIBAQDF5GL7Gjn+UnPFy5sqP2k4XHth +mkWFZj+mjK6cDhbBXYt60NrwVdrrgOFydMC75VeUceFxG/5GD7wrXZP23xzbnWKm +7FxfOSmr4y+1rVEZwi8IeWEz3W6C6y5rjZsCI+pBgdna+aJSpTQZHPfDpNtQm5vl +enj5BfizYixinORxm9kvXMGXV+Cw1CJkqB3mzScwWt40EtQoVxekebf8B7i4ZHyx +xT6/xwF+WY8OliZkY1pdqncoTLUAYcaE/HR/ojJKmSVIq1GswZE/y3E56LIwq+wJ +eGbgH46a+86z+VO2UX1jbad1kWKBCsRoOpaybZDEWAYTOqahW7kOH7umTUsRAgMB +AAGjUzBRMB0GA1UdDgQWBBTIqa3BNEjjcxkhXqnRwInrLM9yijAfBgNVHSMEGDAW +gBTIqa3BNEjjcxkhXqnRwInrLM9yijAPBgNVHRMBAf8EBTADAQH/MA0GCSqGSIb3 +DQEBCwUAA4IBAQB/lYzXb5PI3magMz/IXmwTsMCrVSdaUYEIKLEJggmbGxqpwO1a +iYagWZ/5H3B9KDvNQA+L4FkMJ726ZkdGEH/vkwvTAuhwU2NSWcbRJ8DK5u3Q4rnJ +VswPcW5njUF9mQq0NPX/PMCeOoFDEI8+RrgQZxtHhopwuKOgVA6HRBINKdEZJlrp +oLLQrHDNVLMYTclNHG6kBg0lOHUV31TgkJQ8kMgtq0WQX7RseKR10QKgN5iOBomU +3+y713Ibpac5B1zw5l3LjE/59xFteFbDENr2+A5VGhVNVZC+bs+YTYTzeAsGB0e3 +MQ+XJMJq3kaJmQ+QcTrRaKMtoMz2h1AIRbLd +-----END CERTIFICATE----- +""" + +# The private key that matches TEST_CERT. +TEST_KEY = """\ +-----BEGIN PRIVATE KEY----- +MIIEvAIBADANBgkqhkiG9w0BAQEFAASCBKYwggSiAgEAAoIBAQDF5GL7Gjn+UnPF +y5sqP2k4XHthmkWFZj+mjK6cDhbBXYt60NrwVdrrgOFydMC75VeUceFxG/5GD7wr +XZP23xzbnWKm7FxfOSmr4y+1rVEZwi8IeWEz3W6C6y5rjZsCI+pBgdna+aJSpTQZ +HPfDpNtQm5vlenj5BfizYixinORxm9kvXMGXV+Cw1CJkqB3mzScwWt40EtQoVxek +ebf8B7i4ZHyxxT6/xwF+WY8OliZkY1pdqncoTLUAYcaE/HR/ojJKmSVIq1GswZE/ +y3E56LIwq+wJeGbgH46a+86z+VO2UX1jbad1kWKBCsRoOpaybZDEWAYTOqahW7kO +H7umTUsRAgMBAAECggEAA9SnFFqGfR1yO4XUlfmmgyZqJoJmnl2TdZlDT4bHyrwx +dSIKHO0iiNzEsHMhYHnA62EVdruunUNUdofwE28v9zHZnSbV5mt8OqUyERue5mdf +gvPbjXYXu63LBx61fZH9qME3WwFqUpx7UNGiW62LJ8ktWEC5ywNCNFG+D3YfR3Iu +v3PdIFXwkpJMaXO+42JSoSVoiqxlONqNiqcdQj64iYgCYdNxcgdLy+mHGrf1hAZs +dom2Jp0oEM4ZdOQu3Z6uyEyPsECiz1PdQjzagaEfPWtKCQk0kY8DDCn5X2xQif9w +3xeaISqho7bQgSo4IEgWHTP6V0v7brTP7VcK+gyMyQKBgQD15FrE4XAL89NBQ6iX +m2l6quZv5tIUtreuYxsOpFIMU22zfsRZOvA7sJR3ufOJ+b7tFHuJ8DAJUpK08Vvg +A930/LW9wUsY42d54XKIO/8DTsIrmjppoGdshs3axJQkqfN0zQkhqfbwaXRJ9X2k +Fvax+5jftaIaw0hQdWLnfTmGmQKBgQDOBue89D8THYl/LvDGg6WoVyK3ljBMK2s3 +4BljeZCJdWAjtido13Mltubc6YVHScmVoIZKTmx+fCjdQ3y1t92vZZSBeLsXhfFr +N+IOGZu3ZmJu64x3OukSYQ7x6agi5yP3+7k0siZgxOMXJRQDYZUcHxIHDrSGLeLZ +sj7LnbvLOQKBgE8murE1gEPYsOAJT3O96y45ZQQQYP+Z8XaJIGSOMHsXP/DPlZTD +jCEqrh/8E5EOe48FUN8OGehmVCM6rkBl/kSmNDpoxiu0x9JL5/pClcwSxh4S/0qQ +/7nHiuwo6ycCLgQjHBViCMNKrsw/4bm4SqDwRD1+0jebNOPxZWzuul3BAoGAZij0 +ZjSyxhbCZEdxau5CiYvTkjct8cch3k4IKNRRwGdsaajcN9eFqHDeXzKIPQYwqDo1 +/MiQcdO9K6JYR39JtLxo/B5Sn2JyiJjoRdea6EEjlB7GwyR6B/wKvhf/oHb+1euD +NccU0q0ucf6XwulzV8NsXAWFrHc6YnpJOwwW37kCgYAkEyjUc73jImiTyf/IXVOD +UHlRXZPvwtZUuPGe4RI0Gds97tKnvXnvFsPIRCOGfVzZ8z79DGiQ8TR2a0hgZec1 +Mo3J2dCjlv4Q6ACjHkCA1cmi13OHUPnpaeesrOk+SpEVJfR2k4qRWH4z3oxbPO/q +TDFnjWHSgjkDMrxVHZ8wPg== +-----END PRIVATE KEY----- +""" + +# A perfectly valid RSA key that has nothing to do with TEST_CERT. +UNRELATED_KEY = """\ +-----BEGIN PRIVATE KEY----- +MIIEvQIBADANBgkqhkiG9w0BAQEFAASCBKcwggSjAgEAAoIBAQDE7mosrsdBhdn1 +ZIErYmMuPU69ws51JdyqwtRZlV2uLLby0dOfJpFKZPuBPEqTqDimd9N4FSL0CGjG +zXG23RYd9AbdSoorFORrUxeiPzFnbz4v38srGHckOS9ozKAbPLACUEjWMX1NwAZ+ +wGlSz3cWYTWVztFCUIxpvLVR87PSpTnpdCXIj0EABOc6WoLBLE+v2knZOa63LKg6 +GryInD43CnWFBKpH0gdgWqh+ie3NFMumLR8M3lZq2Mk0EFgVWrxPnJobYvOmavNp +NBR3ZugR4X57c1YFyLkYXQIdhxuYV1ZAY6NgmAIsaiCKFqdksF5AJSV7FZNBM43r +33ae/R5hAgMBAAECggEAIgNYsL9+OEWtU8ooWi0r2q5plXJaVNb1gkPUx+U5sS3V +amKNxbj0UrBW1Scr7U1afXwIOP8TkqkKKb4NpCsS2RkO/3USoKbC3fuTwyjdeFM5 +Hy0sytR2rXm4A8aF57ZnYvrpXZ9eGEnwhT9n4Y7mL2YaSnXWZDkDy3Z1rcIk/p5P +jUo4UQyzD/9Gab1stcBbGv+66B3mlVdRVJor5+tGn6zmr4TjvqupAexuXd+Q6+1L +OG4e2bAUmuOVOa+w8Xo4vwkiXSDLVEsl1z1x4Bcrv61bIg0rbZAeqcZ4EUPyrEyR +RpaaOLAiwdzjOj5pQvHAF2q/++IlY6xPhotTr11tvQKBgQD37C2UQRGRo622+2kx +qgsdA6jPM9vCE9S4aS4qaOj8XSMMCu6bQW5lU48DaONvXxDmT6x9IrgR3QN1Iz/t +17kUCCghuviEP1RjnahBRQqZR38KdDzKhaWt9jgdjCun9MzUXIaQRTFlFGA1z1pz +apZ9a8/ehYPSk4pv9h06/B3IjQKBgQDLWPBuhj203QOmwVF1v1k4yyuJ7UBqtRaY +I9jMW93sB2hPs6Se10UroQHlF95IHeRXvNKH/UXuAILIJMN8oTI4uU7t60shJHvI +o14RzEZoUQjES7BBVhePglndJmYIKoKKAX5lSFe9Lk0ei9B+1Dle5Q+9ZeP09cp4 +vVcGvOgqJQKBgQCtxe1srO8TlhZ821uwY+/GNnpsQX0XW68OUyr4rvAfc2jNWBxG +1mX6v8bOLQa9WXUO+Wl9jIhYfQGfaUW2AC7Jy63VdqgaigksiaUVmr8DEQoK2c6C +ZYrrlFlg3I78+qlXcEMhfF5S6yVEkkJkA6HX52mcHxl2z9OJBokWfwChQQKBgAzH +xzy7FS/D4FHfvpXu89Wc91yQ28aZIRVo01xsvbLy+DxiJwuQrhlC4lKawG656jsV +dAn2AiomQBICNYMkwnpMM0jCzBMGLv16PxRRSW+PAEUOGMLSfWKYp7s9iZYjzdaM +p3wIIvOR8GjmErGV9xEexnF58OzZceNKyyhyQQk9AoGAbvjJvYLOhQpAbbqErNNv +LK1P+TngKpukRHXjiUpPVEGNhN6krBBJCBWzY7ucrIy6jz8UBy7SbITBy7qIKhxZ +PVP7WVATMWEeW1AfdBfCYDI4jFKAD8SLECby45nRBuBllYdQnW1gBzLcCulCwB+w +FV0RvuQPDYkqsx8ibqpSv7c= +-----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 + +# --- stub binaries ----------------------------------------------------------- +STUB_CERTBOT = """\ +#!/bin/sh +# Test stub for certbot: pretend there was nothing to renew. +echo "No renewals were attempted." +exit 0 +""" + +STUB_SOCAT = """\ +#!/bin/sh +# Test stub for socat: record that a reload was attempted, and what was sent. +{ printf 'socat %s <<' "$*"; cat; printf '>>\\n'; } >> "$SOCAT_LOG" +exit 0 +""" + +STUB_HAPROXY = """\ +#!/bin/sh +# Test stub for the haproxy binary: `haproxy -c -f FILE` rejects any config +# containing %(token)s, which is how the tests inject an invalid config. +cfg="" +while [ $# -gt 0 ]; do + case "$1" in -f) cfg="$2"; shift ;; esac + shift +done +if [ -n "$cfg" ] && grep -q '%(token)s' "$cfg" 2>/dev/null; then + echo "[ALERT] parsing [$cfg:1] : unknown keyword '%(token)s'" >&2 + exit 1 +fi +exit 0 +""" % {'token': BROKEN_TOKEN} + +GOOD_HAPROXY_CFG = textwrap.dedent("""\ + global + daemon + defaults + mode http + frontend fe + bind 0.0.0.0:443 ssl crt /etc/haproxy/certs +""") + + +def write(path, content, mode=None): + with open(path, 'w') as fh: + fh.write(content) + if mode is not None: + os.chmod(path, mode) + return path + + +def read(path): + with open(path) as fh: + return fh.read() + + +class CertScriptFixture(unittest.TestCase): + """An isolated fake /etc/haproxy + /etc/letsencrypt plus stub binaries.""" + + def setUp(self): + self.tmp = tempfile.mkdtemp(prefix='haproxy-cert-test-') + self.addCleanup(self._cleanup_tmp) + + self.bindir = os.path.join(self.tmp, 'bin') + os.makedirs(self.bindir) + write(os.path.join(self.bindir, 'certbot'), STUB_CERTBOT, 0o755) + write(os.path.join(self.bindir, 'socat'), STUB_SOCAT, 0o755) + write(os.path.join(self.bindir, 'haproxy'), STUB_HAPROXY, 0o755) + self.socat_log = os.path.join(self.tmp, 'socat-invocations.log') + + # Mirrors the real layout: certs dir, with staging/backups as SIBLINGS. + self.haproxy_dir = os.path.join(self.tmp, 'etc', 'haproxy') + self.certs_dir = os.path.join(self.haproxy_dir, 'certs') + self.staging_dir = os.path.join(self.haproxy_dir, 'cert-staging') + self.backup_dir = os.path.join(self.haproxy_dir, 'cert-backups') + os.makedirs(self.certs_dir) + + self.le_live = os.path.join(self.tmp, 'etc', 'letsencrypt', 'live') + self.domain_dir = os.path.join(self.le_live, DOMAIN) + os.makedirs(self.domain_dir) + self.src_cert = write(os.path.join(self.domain_dir, 'fullchain.pem'), TEST_CERT) + self.src_key = write(os.path.join(self.domain_dir, 'privkey.pem'), TEST_KEY) + + self.live_pem = os.path.join(self.certs_dir, DOMAIN + '.pem') + self.haproxy_cfg = write(os.path.join(self.haproxy_dir, 'haproxy.cfg'), + GOOD_HAPROXY_CFG) + self.log_file = os.path.join(self.tmp, 'haproxy-manager.log') + 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. + for root, dirs, files in os.walk(self.tmp): + for name in files: + try: + os.chmod(os.path.join(root, name), 0o600) + except OSError: + pass + shutil.rmtree(self.tmp, ignore_errors=True) + + # -- helpers --------------------------------------------------------- + def env(self, **overrides): + env = dict(os.environ) + env.update({ + 'PATH': self.bindir + os.pathsep + os.environ['PATH'], + 'SSL_CERTS_DIR': self.certs_dir, + 'LETSENCRYPT_LIVE_DIR': self.le_live, + 'CERT_STAGING_DIR': self.staging_dir, + 'CERT_BACKUP_DIR': self.backup_dir, + 'LOG_FILE': self.log_file, + 'ERROR_LOG_FILE': self.error_log, + 'HAPROXY_CONFIG': self.haproxy_cfg, + 'SOCAT_LOG': self.socat_log, + }) + env.update(overrides) + return env + + def run_script(self, name, **env_overrides): + path = os.path.join(SCRIPTS_DIR, name) + self.assertTrue(os.path.exists(path), f'{path} does not exist') + return subprocess.run(['bash', path], env=self.env(**env_overrides), + capture_output=True, text=True, timeout=120) + + def bundle_is_valid(self, path): + """Ask the shipped library whether HAProxy could use this bundle.""" + return subprocess.run( + ['bash', '-c', '. "$1"; cert_bundle_valid "$2"', '_', LIB, path], + env=self.env(), capture_output=True, text=True).returncode == 0 + + def reload_attempted(self): + return os.path.exists(self.socat_log) and os.path.getsize(self.socat_log) > 0 + + def logs(self): + text = '' + for path in (self.log_file, self.error_log): + if os.path.exists(path): + text += read(path) + return text + + def seed_previous_bundle(self, content=PREVIOUS_BUNDLE): + return write(self.live_pem, content) + + def assert_certs_dir_is_clean(self): + """HAProxy loads this directory wholesale: only final *.pem may be here.""" + entries = sorted(os.listdir(self.certs_dir)) + strays = [e for e in entries if not e.endswith('.pem')] + self.assertEqual(strays, [], + f'non-.pem files left in the certs directory HAProxy ' + f'loads wholesale: {strays} (dir: {entries})') + + def assert_no_staging_leftovers(self): + if os.path.isdir(self.staging_dir): + self.assertEqual(sorted(os.listdir(self.staging_dir)), [], + 'staging file was not cleaned up') + + +class CertScriptBehaviour: + """Behaviour shared by renew-certificates.sh and sync-certificates.sh. + + A mixin rather than a TestCase so the cases are collected once per concrete + script, not a third time for the base class. + """ + + SCRIPT = None + + def run_it(self, **env_overrides): + return self.run_script(self.SCRIPT, **env_overrides) + + # -- happy path ------------------------------------------------------ + def test_publish_replaces_live_pem_and_archives_the_previous_one(self): + self.seed_previous_bundle() + result = self.run_it() + + self.assertEqual(result.returncode, 0, result.stdout + result.stderr) + self.assertEqual(read(self.live_pem), NEW_BUNDLE, + 'live pem is not the newly assembled cert+key') + backup = os.path.join(self.backup_dir, DOMAIN + '.pem') + self.assertTrue(os.path.exists(backup), + f'previous bundle was not archived to {backup}') + self.assertEqual(read(backup), PREVIOUS_BUNDLE, + 'the archived bundle is not the one that was replaced') + self.assertIn('1 updated, 0 failed', self.logs()) + self.assertTrue(self.reload_attempted(), + 'HAProxy was never reloaded after a successful update') + self.assert_certs_dir_is_clean() + self.assert_no_staging_leftovers() + + def test_first_publish_works_with_no_previous_bundle(self): + result = self.run_it() + self.assertEqual(result.returncode, 0, result.stdout + result.stderr) + self.assertEqual(read(self.live_pem), NEW_BUNDLE) + self.assert_certs_dir_is_clean() + + # -- THE HEADLINE ---------------------------------------------------- + def test_unreadable_source_key_leaves_the_previous_bundle_intact(self): + """The bug this whole change exists for. + + Pre-fix: `cat cert key > live.pem` truncated live.pem before cat ran, + so an unreadable key left a key-less (or empty) pem in the directory + HAProxy loads wholesale -> the :443 bind fails -> every site on the + host loses HTTPS. Checking cat's exit status did not undo that. + """ + self.seed_previous_bundle() + before = read(self.live_pem) + self.assertTrue(self.bundle_is_valid(self.live_pem), + 'fixture precondition: the seeded bundle must be valid') + + how = self.break_source_key() + + result = self.run_it() + + self.assertEqual(read(self.live_pem), before, + f'the live pem was damaged by a failed publish ({how}); ' + f'HAProxy would fail to load the certs directory') + self.assertTrue(self.bundle_is_valid(self.live_pem), + 'the pem left on disk is no longer a usable bundle') + logs = self.logs() + self.assertIn(f'Failed to combine certificate for {DOMAIN}', logs) + self.assertIn('0 updated, 1 failed', logs) + self.assertFalse(self.reload_attempted(), + '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') + + def break_source_key(self): + """Make reading the source key fail, however this environment allows. + + chmod 000 is the faithful reproduction (file present, `-f` true, cat + fails), but it is a no-op for root, so as root we truncate the key + instead: pre-fix that is even nastier, because `cat` then *succeeds* + and silently publishes a key-less pem. + """ + if os.geteuid() == 0: + write(self.src_key, '') + return 'zero-length source key (running as root)' + os.chmod(self.src_key, 0o000) + return 'unreadable source key (chmod 000)' + + # -- other ways to end up with an unusable bundle -------------------- + def test_cert_only_bundle_is_rejected(self): + self.seed_previous_bundle() + before = read(self.live_pem) + write(self.src_key, TEST_CERT) # no private key block at all + + self.run_it() + + self.assertEqual(read(self.live_pem), before, + 'a key-less bundle was published over the live pem') + self.assertIn('0 updated, 1 failed', self.logs()) + self.assertFalse(self.reload_attempted()) + self.assert_certs_dir_is_clean() + self.assert_no_staging_leftovers() + + def test_truncated_certificate_block_is_rejected(self): + self.seed_previous_bundle() + before = read(self.live_pem) + write(self.src_cert, TEST_CERT.split('\n')[0] + '\nMIIDFzCCAf+gAwIBA\n') + + self.run_it() + + self.assertEqual(read(self.live_pem), before, + 'a truncated certificate was published over the live pem') + self.assertIn('0 updated, 1 failed', self.logs()) + self.assert_certs_dir_is_clean() + + 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.seed_previous_bundle() + before = read(self.live_pem) + write(self.src_key, UNRELATED_KEY) + + self.run_it() + + self.assertEqual(read(self.live_pem), before, + 'a bundle whose key does not match the cert was published') + self.assertIn('does not match the certificate', self.logs()) + self.assertIn('0 updated, 1 failed', self.logs()) + self.assert_certs_dir_is_clean() + + # -- the certs directory is HAProxy's, not ours ---------------------- + def test_no_stray_files_in_certs_dir_after_success_or_failure(self): + self.seed_previous_bundle() + self.run_it() + self.assert_certs_dir_is_clean() + self.assertEqual(sorted(os.listdir(self.certs_dir)), [DOMAIN + '.pem']) + + self.break_source_key() + self.run_it() + self.assert_certs_dir_is_clean() + self.assertEqual(sorted(os.listdir(self.certs_dir)), [DOMAIN + '.pem']) + self.assert_no_staging_leftovers() + + def test_staging_and_backup_dirs_are_outside_the_certs_dir(self): + """Belt and braces: even with the defaults, nothing lands under certs/.""" + self.seed_previous_bundle() + # Drop the explicit overrides so the derived defaults are exercised. + env = {'CERT_STAGING_DIR': '', 'CERT_BACKUP_DIR': ''} + self.run_it(**env) + + self.assert_certs_dir_is_clean() + self.assertEqual(sorted(os.listdir(self.certs_dir)), [DOMAIN + '.pem']) + self.assertTrue( + os.path.exists(os.path.join(self.haproxy_dir, 'cert-staging')), + 'default staging dir is not the documented sibling of the certs dir') + self.assertTrue( + os.path.exists(os.path.join(self.haproxy_dir, 'cert-backups', + DOMAIN + '.pem')), + 'default backup dir is not the documented sibling of the certs dir') + + # -- reload gating --------------------------------------------------- + def test_reload_is_not_attempted_when_haproxy_config_is_invalid(self): + write(self.haproxy_cfg, GOOD_HAPROXY_CFG + BROKEN_TOKEN + '\n') + + result = self.run_it() + + self.assertFalse(self.reload_attempted(), + 'HAProxy was reloaded with a configuration that ' + '`haproxy -c` rejects') + self.assertNotEqual(result.returncode, 0, + 'refusing to reload must be a loud, non-zero exit') + self.assertIn('does not validate', self.logs()) + + def test_reload_happens_when_the_config_validates(self): + self.run_it() + self.assertTrue(self.reload_attempted()) + self.assertIn('reload', read(self.socat_log)) + + def test_reload_is_not_attempted_when_nothing_was_updated(self): + shutil.rmtree(self.domain_dir) + result = self.run_it() + self.assertEqual(result.returncode, 0) + self.assertFalse(self.reload_attempted()) + + +class TestRenewCertificates(CertScriptBehaviour, CertScriptFixture): + SCRIPT = 'renew-certificates.sh' + + +class TestSyncCertificates(CertScriptBehaviour, CertScriptFixture): + SCRIPT = 'sync-certificates.sh' + + +class TestCertPublishLibrary(CertScriptFixture): + """Unit-level checks on cert-publish-lib.sh itself.""" + + def call(self, snippet, *args): + return subprocess.run( + ['bash', '-c', '. "$1"; shift; ' + snippet, '_', LIB, *args], + env=self.env(), capture_output=True, text=True) + + def test_valid_bundle_accepted(self): + path = write(os.path.join(self.tmp, 'ok.pem'), NEW_BUNDLE) + self.assertEqual(self.call('cert_bundle_valid "$1"', path).returncode, 0) + + 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) + missing = os.path.join(self.tmp, 'nope.pem') + self.assertNotEqual(self.call('cert_bundle_valid "$1"', missing).returncode, 0) + + 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) + + def test_a_broken_live_pem_does_not_overwrite_a_good_backup(self): + """Mirrors create_backup(require_valid=True) in haproxy_manager.py. + + If the pem currently on disk is already garbage, archiving it would + replace a restorable backup with an unusable one. + """ + os.makedirs(self.backup_dir) + good_backup = write(os.path.join(self.backup_dir, DOMAIN + '.pem'), + PREVIOUS_BUNDLE) + write(self.live_pem, 'garbage, not a pem at all\n') + + 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(read(good_backup), PREVIOUS_BUNDLE, + '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.""" + 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') + + 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.assertEqual(read(self.live_pem), before, + 'the live pem was damaged by a failed rename') + self.assert_no_staging_leftovers() + + 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') + os.makedirs(fake_path) + for tool in ('cat', 'grep', 'mktemp', 'mv', 'cp', 'rm', 'mkdir', + 'basename', 'dirname', 'find', 'date', 'chmod'): + real = shutil.which(tool) + if real: + os.symlink(real, os.path.join(fake_path, tool)) + path = write(os.path.join(self.tmp, 'ok.pem'), NEW_BUNDLE) + + # bash by absolute path: the stripped PATH cannot resolve it. + result = subprocess.run( + [shutil.which('bash'), '-c', '. "$1"; cert_bundle_valid "$2"', + '_', 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') + + 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) + + # bash by absolute path: the stripped PATH cannot resolve it. + result = subprocess.run( + [shutil.which('bash'), '-c', '. "$1"; cert_bundle_valid "$2"', + '_', LIB, path], + env=self.env(PATH=fake_path), capture_output=True, text=True) + + self.assertNotEqual(result.returncode, 0, + 'structural checks stopped being mandatory') + + +class TestScriptsAreSane(unittest.TestCase): + """Cheap static guards against the failure mode coming back.""" + + SHELL_FILES = ('renew-certificates.sh', 'sync-certificates.sh', + 'cert-publish-lib.sh') + + def test_shell_files_parse(self): + for name in self.SHELL_FILES: + path = os.path.join(SCRIPTS_DIR, name) + result = subprocess.run(['bash', '-n', path], + capture_output=True, text=True) + self.assertEqual(result.returncode, 0, + f'{name}: {result.stderr}') + + def test_no_script_redirects_into_the_live_pem(self): + pattern = re.compile(r'>\s*"?\$\{?COMBINED_FILE') + for name in ('renew-certificates.sh', 'sync-certificates.sh'): + body = read(os.path.join(SCRIPTS_DIR, name)) + self.assertIsNone(pattern.search(body), + f'{name} still redirects output straight into the ' + f'live pem HAProxy is serving') + + +if __name__ == '__main__': + print(f'testing scripts from: {SCRIPTS_DIR}') + unittest.main(verbosity=2) diff --git a/scripts/test-cert-write-safety.py b/scripts/test-cert-write-safety.py new file mode 100755 index 0000000..9125f53 --- /dev/null +++ b/scripts/test-cert-write-safety.py @@ -0,0 +1,737 @@ +#!/usr/bin/env python3 +"""Regression tests for certificate bundle publishing. + +Why this file exists +-------------------- +Every code path that refreshed a combined PEM used to do this: + + with open(combined_path, 'w') as combined: # TRUNCATES + subprocess.run(['cat', cert, key], stdout=combined) # rc ignored + +`combined_path` is the live bundle HAProxy is serving. open(..., 'w') empties it +BEFORE any source material has been read, and the `cat` exit status was never +checked, so a half-written certbot lineage, an unreadable source or a full disk +left a truncated or key-less PEM in place. HAProxy loads /etc/haproxy/certs as a +directory and refuses to start if any file in it is unusable, so that is HTTPS +down for every site on the host - and unlike a broken haproxy.cfg it is not +recoverable by config rollback. + +The bundle endpoint made it worse: it deleted superseded .pem files AND ran +`certbot delete` on their lineages before anything had checked that the +replacement was usable, destroying both copies of a working certificate. +Recovery there means fresh, rate-limited ACME orders. + +These tests pin the invariants: + * a failed publish leaves the previously served bundle byte-for-byte intact + and the edge still able to start; + * nothing is published that is not a complete, validated cert+key pair; + * no old certificate file is removed and no lineage deleted until the + replacement is validated, in place, and actually loaded by HAProxy; + * only final .pem files ever exist in the crt directory. + +Running +------- + python3 scripts/test-cert-write-safety.py # tests the repo checkout + HAPROXY_MANAGER_DIR=/some/other/tree \ + python3 scripts/test-cert-write-safety.py # tests another tree + +Pointing HAPROXY_MANAGER_DIR at a pre-fix checkout is how the bugs above were +reproduced: the behavioural tests run there too (the fix-only tests skip +themselves), and they fail. + +Same conventions as scripts/test-config-rollback.py: self-contained stdlib +unittest, no pytest/venv/extra dependencies, stub binaries on PATH. The +certificate material below is real (a self-signed leaf plus its matching key, +and one unrelated key for the mismatch case) and embedded as constants so the +suite needs no crypto tooling to create fixtures. +""" + +import logging +import os +import shutil +import sqlite3 +import subprocess +import sys +import tempfile +import textwrap +import unittest + +BROKEN_TOKEN = '__BROKEN__' + +MODULE_DIR = os.path.abspath( + os.environ.get('HAPROXY_MANAGER_DIR', + os.path.join(os.path.dirname(os.path.abspath(__file__)), '..')) +) + +# haproxy_manager builds its Jinja2 environment from the relative path +# Path('templates'), so it has to be imported with the module dir as cwd. +os.chdir(MODULE_DIR) +sys.path.insert(0, MODULE_DIR) + +# The module opens /var/log/haproxy-manager.log at import time via +# logging.FileHandler. Redirect that one call so the suite runs unprivileged. +_LOG_DIR = tempfile.mkdtemp(prefix='haproxy-mgr-test-logs-') +_real_file_handler = logging.FileHandler +logging.FileHandler = ( + lambda fn, *a, **kw: _real_file_handler( + os.path.join(_LOG_DIR, os.path.basename(fn)), *a, **kw) +) +try: + import haproxy_manager as hm +except ImportError as exc: # pragma: no cover - environment problem, not a failure + sys.stderr.write( + f"SKIP: cannot import haproxy_manager ({exc}).\n" + "Install the application requirements first: pip install -r requirements.txt\n" + ) + raise SystemExit(77) +finally: + logging.FileHandler = _real_file_handler + +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)') +NEEDS_OPENSSL = unittest.skipUnless( + shutil.which('openssl'), 'needs the openssl CLI') + +# --- real test material ----------------------------------------------------- +# Self-signed leaf, CN=test.example.com, SAN test.example.com + +# www.test.example.com, valid until 2126. +LEAF_CERT = """\ +-----BEGIN CERTIFICATE----- +MIIDTjCCAjagAwIBAgIUTliK3dNIdYS3i7R3yxNMHfxVWTcwDQYJKoZIhvcNAQEL +BQAwGzEZMBcGA1UEAwwQdGVzdC5leGFtcGxlLmNvbTAgFw0yNjA4MDYxNTQxMjda +GA8yMTI2MDcxMzE1NDEyN1owGzEZMBcGA1UEAwwQdGVzdC5leGFtcGxlLmNvbTCC +ASIwDQYJKoZIhvcNAQEBBQADggEPADCCAQoCggEBAMRuorQDGdsyc/rk3WST8J70 +9oVcTz2QSTQ5QNxqa4jKX7vD0YdLK2Kz7TKLbR//ju+dnqFhLTAs38KqljmU5M1d +7hC1frSV9Y9heHTa51fc9hxewDl9535TdIsUga5HT+sc7q3Np7RparOf1NOm/aBd +27j+LGKclbJ5YaU3I39S05H+S8mgmFpLezwQ7uzFomkk/E5deUcqGpJrW8k2t6Uv +jvfejB8FGAM6DxEL4yTjizmIaJE+jadPTJRq1TtHG+LE4rt2UpF4ggNFXlK7xXfp +p0mcIE1b3MwGaPGmAe5Vw2+nl4uD9LRgQh3XxO2+as4QTTYhlR40Fn03ksXVo6cC +AwEAAaOBhzCBhDAdBgNVHQ4EFgQUjAt+yg/dpNHyeWNqH2oAURfW3iUwHwYDVR0j +BBgwFoAUjAt+yg/dpNHyeWNqH2oAURfW3iUwDwYDVR0TAQH/BAUwAwEB/zAxBgNV +HREEKjAoghB0ZXN0LmV4YW1wbGUuY29tghR3d3cudGVzdC5leGFtcGxlLmNvbTAN +BgkqhkiG9w0BAQsFAAOCAQEABo5x4n3/61XxwJEkNPyv/mCAN5t/+NrMxfadRFJK ++jBxtOO8w8vhi21zF1NDVQkt2bt69QGrVleP5X78FaIaI6sJpLKDE1nOyE9Dpt4y +mnLoRi0Ep7NaDV6rHmfbokLkVdd4Z9RKUESnYCc1Zt5x82oGhEe3GJ4ej2HS8sGY +r79qGVEhQIbLPDA3PD+RQCF6+xNU2CVgZUJ7ZtSeAaNaQQqTRUT2qCBvIKfV8fMS +VDmV6/YORV2jTzO3odKsKxXQY8oWCzfwosxD0dJ2zbZWvMAqcQQq3d/iuH8NiS4Y ++zZ6j9KI/RdRYJz/co2FKAy3FfQ/gZo8eWF2gf9RUDgNQg== +-----END CERTIFICATE----- +""" + +LEAF_KEY = """\ +-----BEGIN PRIVATE KEY----- +MIIEvgIBADANBgkqhkiG9w0BAQEFAASCBKgwggSkAgEAAoIBAQDEbqK0AxnbMnP6 +5N1kk/Ce9PaFXE89kEk0OUDcamuIyl+7w9GHSytis+0yi20f/47vnZ6hYS0wLN/C +qpY5lOTNXe4QtX60lfWPYXh02udX3PYcXsA5fed+U3SLFIGuR0/rHO6tzae0aWqz +n9TTpv2gXdu4/ixinJWyeWGlNyN/UtOR/kvJoJhaS3s8EO7sxaJpJPxOXXlHKhqS +a1vJNrelL4733owfBRgDOg8RC+Mk44s5iGiRPo2nT0yUatU7RxvixOK7dlKReIID +RV5Su8V36adJnCBNW9zMBmjxpgHuVcNvp5eLg/S0YEId18TtvmrOEE02IZUeNBZ9 +N5LF1aOnAgMBAAECggEAR0NcA7KcTsmfCga9yx9gzEpSpU838D3IUQn0XgK9wIKq ++JOyEENVGhnsk8nBbTppwMSOKD35BuFAzH7WwU0jNN4+4BD4RsugqsPRz5MbGuUu +5Fv7oN/sfAgK3+owoel9NO7qKGPT07/q1f/GVoLewK9MZ3DO6XelV3px0l6OokHn +Oy+ShWw4CnUXRIAKBuRNLLA0HQ4ld0dceV/kT428tZIluj9pkUhnp9xux5yDWkMu +dj26CXZxp2PMzVLD21TmI/BZO+3Ev+Nf5GaS8GrmROlXLKTE5YDmU1ZTECsSSFLK +Nuw7B3TT+xEQUf5lokDigkbTgIQ5kpnIy1LSbqLAAQKBgQDudSbhaqABZk6B09UL +PZVQZSrGZDXl4vwnkHXArrr2BRz1+B67tWebq3V0pCrZqwFc9eOtq1F0vH6q11Ns +vLZ9lZ7KUEsFS6InZkqSQSGrgWt4MIjocc8qHLlaEk/EGgHgtwBblm2hEyDwy487 +jhnc2wI82D6ZLvIc27pQQFNjAQKBgQDS4gXINPsaq9YFiQ9JQG8oJGSq6yn137wJ +DQYrTaB9tqcmIXWx6ZgMiKZiOGLQYVmlNFoh10cTxbebWjXjWL4VP3gg5nE87pQz +8KKvkXP07QWiSQLV5SJzEeDM/l3Lkyc9O69PYnusWjdFOrIPDTmxZ2l6wNyAVxpm +zEK17gYOpwKBgQC+glxAxZX16E2ajanspBPRujG1dMRW2MTJuzFIcpCuEyGzJbsw +DlsrVI2vVaViZ6vcIBr5WiDm2d19EjD1c8N8i/fj/Mgi/+0Z+zBirqR+yBQbXvNS +efKf23j+DBksO/b6GFqx0XnesVCk8IyLcRkaiOK9x6ojag1Gnwm4Kdw1AQKBgAen +ssQIwFDAih1bU1W6ZA6V+52Eudo2C/JcKawqvjeyCLFGp6oUq7NQxpFsMJIV5pYr +p1XxJaBfHgIirTAaiZPl4Ot40gV/N5wHETDEW+w5Kmowskyna6+3p2xpk2gPaG49 +m2iLT6f7AmSd89a+CSkacubE13xFLS0sHwPRpyCjAoGBAJfFYXWFAD2viAmBshCS +g6Ba78n0vkF74/DnFRWpIqxw8vufQ5nY/k67tGko+zZg3jzI1d1REvI1qHYctKFM +Ko7fmF3ny803cdJT8EuIlrU2+V5lh0GkPO/or+68qso+qBj3R8f6jRLbahkUbY5e +yR3PzoVVfGaftvLsdbAqD+VC +-----END PRIVATE KEY----- +""" + +# A perfectly valid key that simply does not belong to LEAF_CERT. +UNRELATED_KEY = """\ +-----BEGIN PRIVATE KEY----- +MIIEvwIBADANBgkqhkiG9w0BAQEFAASCBKkwggSlAgEAAoIBAQDBpZAkTBLhiDnq +cCjsSsBwAW/fhYYfx+t2iauYPrvaLmiJHFkXEr7gaBlYWTKJRISY2jqYTM1RdLJB +DDjNThrewOLtcF7d+k+ArMPWXxqBotbCTKMkh7djRfnXEcjJ/mil31W351cR/11q +EPBdCcj4BYegBzc4GG2qIh/Nww+jd1KkkxUhnTMPp5Ie0myJafh0Pdsss9lbmqBH +fW6BO341C9jG8N/7C7FHCzzf74q7mf0Bx+isYgW/1YL0Ndg/R8yTRVKTbPvU3ZM2 +hEeLDXFbvcceREq1j0VEXGd9rcB2JHBE4TAQZq9WmeT51uynW8a1hnUcUs/li7jL +cuI6DswxAgMBAAECggEADeC4Z42HJeAeLHG80RBbYbuMobN/RPxOIOjlYgwO6Og+ +CCN+tANdKBZ1yInd8A33xb+QBvWsGjwXgUdns7j2/oNK0BLnTZfAhlt7Tnvy2ZsK +spKM95N9XlE3wkTNQ8Kmi8qpaTxcVlcbgfw0SaqnmzTEP0D9IVlI1LJM3rFtx7xn +xhrtKzhcv6WbnZKjrKlPpJq7dFC8+3WtbWFBvWFUTs2mQJ3eFWlc/EBh60PKuMAz +44aMrypzQSdcAs0+f0fOpnhpWa6Px4uEAIvSNPjPI7gt/7v4qb+fNrGGYrou9Pd9 +hW0MGDOxfTxr094YCdapklIV3OyCvrCeS8XVYnVddQKBgQDiZzBTNnc6O2EWgY2h +82VSEBMDTnk+rT7CTx4RGkzyq2z6oUg0itr+TQGhyjRX24VbROXtXlCBNXQkn9UA +aZsss+KnrF6KLEmdwQIoNKSgBIiTgX/PHmkf3a+scDShgL5aIItjazu9TEblBqKF ++H4eBwuPjKc20h2CctoZswE0HQKBgQDa9iZBOPCjyTtq4oSeJbyvjYfJN5EoI9nZ +hl0Yqa8ajbJ8nyxGziy5z5ktqBFYiVa53xamagJJp69DCmnm6vSy01KZIOtKNEKb +PCaNc1Lp+cf5SEIHX0Pakx/zmi0PzDx1V0DhtjLhF8dFUfQhWvvxF+m16LZeBYZ+ +0UP6NRAUJQKBgQCM/e/1UkzrocDzkBiQy4/EjCga/gq5gpA716OEyRk0YpdKeZgK +yJJancAvbkoskJO64+xAZ2TBInXCvRqb2Ch/rUKwYsK5T51EtcbPHQGMeWZIXfQn +GuwioR7exz2vegqQ/AVyE3yvhUn9JKWfwsFfl8mWSuRzWmRwMXArYvOT7QKBgQCV +6Y2LfjaTjMUXivsNY/zpnNbo1xiVCOawXaQDrLlsTrNzS29/Es3gcdgIQFeP7Ifq +Pmk9irsCPsJp/gk/xoG+pZyZpsYxSdKIgghLNDgCZbeaXvSGI51LWwu3N0m+1TBX +jmOnpZz0K9mNBm1FIQv5p0ul9ixV9yZ8UT5fYlEd2QKBgQDLWMgMD9rOORl+0s8Z +RcpSfu2E7KP0e2DaxP4dYRmUyNuE8iK8hglNqqVBLhrFHmDZ1yo1lFIDBROaJ9EW +pAxSb8kRi2dba8ZnpEDhhiqoDwkoXRDOrzd5bdHyiOJV39U1F5U6HqDE4URaq8iu +k49ABlblWHBsoUF63ka1PBrMgA== +-----END PRIVATE KEY----- +""" + +GOOD_BUNDLE = LEAF_CERT + LEAF_KEY + +# 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 +# one that is not takes the whole listener (i.e. the whole edge) down. That is +# what makes "the edge would still start" an assertion rather than a hope. +FAKE_HAPROXY = textwrap.dedent(f"""\ + #!/bin/sh + # haproxy -c -f FILE -> reject FILE containing {BROKEN_TOKEN} + # reject any unusable file in $TEST_CERTS_DIR + cfg="" + while [ $# -gt 0 ]; do + case "$1" in -f) cfg="$2"; shift ;; esac + shift + done + if [ -n "$cfg" ] && grep -q '{BROKEN_TOKEN}' "$cfg" 2>/dev/null; then + echo "[ALERT] parsing [$cfg:1] : unknown keyword '{BROKEN_TOKEN}'" >&2 + exit 1 + fi + if [ -n "$TEST_CERTS_DIR" ] && [ -d "$TEST_CERTS_DIR" ]; then + for f in "$TEST_CERTS_DIR"/*; do + [ -e "$f" ] || continue + if [ ! -s "$f" ]; then + echo "[ALERT] unable to load SSL certificate from empty file '$f'" >&2 + exit 1 + fi + if ! grep -q -- '-----END CERTIFICATE-----' "$f"; then + echo "[ALERT] unable to load SSL certificate from '$f'" >&2 + exit 1 + fi + if ! grep -q -- '-----END .*PRIVATE KEY-----' "$f"; then + echo "[ALERT] unable to load SSL private key from '$f'" >&2 + exit 1 + fi + done + fi + exit 0 +""") + +# certbot stub: succeeds, announces a renewal, and records every invocation so +# tests can assert that `certbot delete` did or did not run. +FAKE_CERTBOT = textwrap.dedent("""\ + #!/bin/sh + if [ -n "$TEST_CERTBOT_LOG" ]; then + echo "$@" >> "$TEST_CERTBOT_LOG" + fi + case "$1" in + renew) echo "Congratulations, all renewals succeeded" ;; + delete) [ -n "$TEST_CERTBOT_DELETE_FAILS" ] && exit 1 ;; + esac + exit 0 +""") + +# socat stub: records reload attempts so tests can assert HAProxy was NOT +# reloaded with unvalidated material. +FAKE_SOCAT = textwrap.dedent("""\ + #!/bin/sh + if [ -n "$TEST_SOCAT_LOG" ]; then + echo "$@" >> "$TEST_SOCAT_LOG" + fi + cat > /dev/null 2>&1 + exit 0 +""") + + +def structurally_valid(text): + """Is this text a usable cert+key bundle? + + Deliberately implemented here rather than calling into haproxy_manager, so + the assertions stay honest when the suite is pointed at a tree whose + validation code is the thing under test (or absent entirely). + """ + return ('-----BEGIN CERTIFICATE-----' in text + and '-----END CERTIFICATE-----' in text + and any(f'-----END {label}-----' in text for label in + ('PRIVATE KEY', 'RSA PRIVATE KEY', 'EC PRIVATE KEY'))) + + +class CertPublishTestCase(unittest.TestCase): + """Isolated fake /etc/haproxy + /etc/letsencrypt plus stub binaries.""" + + def setUp(self): + self.tmp = tempfile.mkdtemp(prefix='haproxy-cert-test-') + self.addCleanup(shutil.rmtree, self.tmp, True) + + bindir = os.path.join(self.tmp, 'bin') + os.makedirs(bindir) + for name, body in (('haproxy', FAKE_HAPROXY), + ('certbot', FAKE_CERTBOT), + ('socat', FAKE_SOCAT)): + path = os.path.join(bindir, name) + with open(path, 'w') as fh: + fh.write(body) + os.chmod(path, 0o755) + + self.certbot_log = os.path.join(self.tmp, 'certbot-invocations.log') + self.socat_log = os.path.join(self.tmp, 'socat-invocations.log') + self.etc = os.path.join(self.tmp, 'etc') + self.certs = os.path.join(self.etc, 'certs') + os.makedirs(self.certs) + + self._saved_env = dict(os.environ) + os.environ['PATH'] = bindir + os.pathsep + os.environ['PATH'] + os.environ['TEST_CERTS_DIR'] = self.certs + os.environ['TEST_CERTBOT_LOG'] = self.certbot_log + os.environ['TEST_SOCAT_LOG'] = self.socat_log + self.addCleanup(self._restore_env) + + overrides = { + 'DB_FILE': os.path.join(self.etc, 'haproxy_config.db'), + 'HAPROXY_CONFIG_PATH': os.path.join(self.etc, 'haproxy.cfg'), + 'HAPROXY_BACKUP_PATH': os.path.join(self.etc, 'haproxy.cfg.backup'), + 'BLOCKED_IPS_MAP_PATH': os.path.join(self.etc, 'blocked_ips.map'), + 'BLOCKED_IPS_MAP_BACKUP_PATH': os.path.join(self.etc, 'blocked_ips.map.backup'), + 'CORAZA_SPOE_CONFIG_PATH': os.path.join(self.etc, 'coraza-spoe.cfg'), + 'CORAZA_SPOE_BACKUP_PATH': os.path.join(self.etc, 'coraza-spoe.cfg.backup'), + 'CLUSTER_SECRET_PATH': os.path.join(self.etc, 'cluster-secret'), + 'SSL_CERTS_DIR': self.certs, + 'HAPROXY_SOCKET_PATH': os.path.join(self.etc, 'haproxy.sock'), + 'API_KEY': None, + } + self._saved = {} + for name, value in overrides.items(): + self._saved[name] = getattr(hm, name, None) + setattr(hm, name, value) + self.addCleanup(self._restore_globals) + + # find_certbot_live_dir() resolves /etc/letsencrypt/live, which we + # cannot repoint on older trees. Stubbing this one lookup keeps the + # suite runnable against a pre-fix checkout for bug reproduction; every + # line of code under test is downstream of it. + self.le_live = os.path.join(self.tmp, 'letsencrypt', 'live') + os.makedirs(self.le_live) + self._real_find = hm.find_certbot_live_dir + hm.find_certbot_live_dir = self._fake_find_live_dir + self.addCleanup( + lambda: setattr(hm, 'find_certbot_live_dir', self._real_find)) + + # log_operation() appends to a hardcoded /var/log path. Injecting `open` + # into the module namespace shadows the builtin for that module only. + real_open = open + log_dir = self.tmp + + def _redirecting_open(path, *args, **kwargs): + if isinstance(path, str) and path.startswith('/var/log/'): + path = os.path.join(log_dir, os.path.basename(path)) + return real_open(path, *args, **kwargs) + + hm.open = _redirecting_open + self.addCleanup(lambda: hm.__dict__.pop('open', None)) + + hm.init_db() + self.client = hm.app.test_client() + + def _restore_env(self): + os.environ.clear() + os.environ.update(self._saved_env) + + def _restore_globals(self): + for name, value in self._saved.items(): + if value is None: + setattr(hm, name, None) + else: + setattr(hm, name, value) + + def _fake_find_live_dir(self, *args, **kwargs): + base = args[0] if args else kwargs.get('base_domain') + path = os.path.join(self.le_live, base) + return path if os.path.isdir(path) else None + + # -- helpers --------------------------------------------------------- + def write(self, path, text): + os.makedirs(os.path.dirname(path), exist_ok=True) + with open(path, 'w') as fh: + fh.write(text) + return path + + def read(self, path): + with open(path) as fh: + return fh.read() + + def make_lineage(self, domain, cert=LEAF_CERT, key=LEAF_KEY): + """Create a certbot live directory. key=None omits privkey.pem.""" + live = os.path.join(self.le_live, domain) + os.makedirs(live, exist_ok=True) + self.write(os.path.join(live, 'fullchain.pem'), cert) + if key is not None: + self.write(os.path.join(live, 'privkey.pem'), key) + return live + + def publish_live_bundle(self, domain): + """A good bundle already being served for `domain`.""" + return self.write(os.path.join(self.certs, f'{domain}.pem'), GOOD_BUNDLE) + + def add_domain(self, domain, backend_name, ssl_cert_path=None): + with sqlite3.connect(hm.DB_FILE) as conn: + cur = conn.cursor() + cur.execute( + 'INSERT INTO domains (domain, ssl_enabled, ssl_cert_path) ' + 'VALUES (?, ?, ?)', + (domain, 1 if ssl_cert_path else 0, ssl_cert_path)) + domain_id = cur.lastrowid + cur.execute('INSERT INTO backends (name, domain_id) VALUES (?, ?)', + (backend_name, domain_id)) + backend_id = cur.lastrowid + cur.execute( + 'INSERT INTO backend_servers ' + '(backend_id, server_name, server_address, server_port) ' + 'VALUES (?, ?, ?, ?)', (backend_id, 'srv1', '10.0.0.1', 8080)) + conn.commit() + + def certbot_invocations(self): + if not os.path.exists(self.certbot_log): + return [] + return [line.strip() for line in self.read(self.certbot_log).splitlines()] + + def certbot_deletes(self): + return [c for c in self.certbot_invocations() if c.startswith('delete')] + + def edge_would_start(self): + """Would HAProxy load the current config + crt directory?""" + return subprocess.run( + ['haproxy', '-c', '-f', hm.HAPROXY_CONFIG_PATH], + capture_output=True).returncode == 0 + + def assert_only_final_pems_in_certs_dir(self): + strays = [n for n in os.listdir(self.certs) if not n.endswith('.pem')] + self.assertEqual( + [], strays, + 'HAProxy loads every file in the crt directory; only final .pem ' + f'bundles may exist there, found: {strays}') + + +class TestLivePemIsNeverTruncated(CertPublishTestCase): + """Bug 1: the live PEM was opened in truncate mode before any source read.""" + + def test_failed_renewal_leaves_previous_bundle_intact(self): + """HEADLINE: a failure mid-publish must not disturb what is served. + + The renewed lineage has a privkey.pem that exists but is empty - the + signature of a write that died half way, and the case the old + `os.path.exists(key)` guard waved straight through into + `cat fullchain emptykey > livepem`. + """ + cert_path = self.publish_live_bundle('renew.example.com') + before = self.read(cert_path) + self.add_domain('renew.example.com', 'renew_backend', + ssl_cert_path=cert_path) + self.make_lineage('renew.example.com', key='') + + resp = self.client.post('/api/certificates/renew') + + after = self.read(cert_path) + self.assertEqual(before, after, + 'the live PEM must still be the previous bundle, ' + 'byte for byte') + self.assertTrue(structurally_valid(after), + 'the served bundle must still contain a cert AND a key') + self.assertTrue(self.edge_would_start(), + 'HAProxy must still be able to load the crt directory') + self.assertEqual(500, resp.status_code, + 'a failed publish must be reported loudly, not as success') + self.assert_only_final_pems_in_certs_dir() + + def test_missing_source_key_leaves_previous_bundle_intact(self): + """Same invariant when privkey.pem is absent rather than empty.""" + cert_path = self.publish_live_bundle('gone.example.com') + before = self.read(cert_path) + self.add_domain('gone.example.com', 'gone_backend', + ssl_cert_path=cert_path) + self.make_lineage('gone.example.com', key=None) + + self.client.post('/api/certificates/renew') + + self.assertEqual(before, self.read(cert_path)) + self.assertTrue(self.edge_would_start()) + + def test_issuance_failure_leaves_previous_bundle_intact(self): + """/api/ssl re-issuing over an existing bundle must be all-or-nothing.""" + cert_path = self.publish_live_bundle('issue.example.com') + before = self.read(cert_path) + self.add_domain('issue.example.com', 'issue_backend') + self.make_lineage('issue.example.com', key='') + + resp = self.client.post('/api/ssl', json={'domain': 'issue.example.com'}) + + self.assertEqual(before, self.read(cert_path)) + self.assertTrue(structurally_valid(self.read(cert_path))) + self.assertTrue(self.edge_would_start()) + self.assertEqual(500, resp.status_code) + self.assertEqual('error', resp.get_json()['status']) + + def test_successful_renewal_still_publishes(self): + """The guard must not block the normal path.""" + cert_path = self.publish_live_bundle('ok.example.com') + 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) + + 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))) + self.assertTrue(self.edge_would_start()) + self.assert_only_final_pems_in_certs_dir() + + +class TestOldCertificateIsNotDestroyedFirst(CertPublishTestCase): + """Bug 2: superseded .pem removed and lineage deleted before validation.""" + + def _setup_supersede(self, broken=True): + # An older single-SAN file whose CN is covered by the new bundle. + old_path = self.write( + os.path.join(self.certs, 'www.test.example.com.pem'), GOOD_BUNDLE) + self.add_domain('test.example.com', 'bundle_backend') + self.make_lineage('test.example.com', key='' if broken else LEAF_KEY) + return old_path + + @NEEDS_OPENSSL + def test_failed_bundle_does_not_remove_the_old_certificate(self): + old_path = self._setup_supersede(broken=True) + + resp = self.client.post('/api/ssl/bundle', json={ + 'primary': 'test.example.com', + 'sans': ['www.test.example.com'], + }) + + self.assertEqual(500, resp.status_code) + self.assertTrue( + os.path.exists(old_path), + 'the superseded certificate must survive a failed replacement - ' + 'it may be the only working copy left') + self.assertTrue(structurally_valid(self.read(old_path))) + self.assertEqual( + [], self.certbot_deletes(), + '`certbot delete` is irreversible and rate-limited to recover from; ' + 'it must never run for a replacement that was never published') + self.assertTrue(self.edge_would_start()) + + @FIX_ONLY + @NEEDS_OPENSSL + def test_lineage_is_deleted_only_after_haproxy_loads_the_bundle(self): + """A reload failure must leave the lineage intact and the file recoverable.""" + old_path = self._setup_supersede(broken=False) + # Make generate_config() produce a config the validator rejects, so the + # publish succeeds but HAProxy never loads it. + self.add_domain('other.example.com', BROKEN_TOKEN + '_backend') + + resp = self.client.post('/api/ssl/bundle', json={ + 'primary': 'test.example.com', + 'sans': ['www.test.example.com'], + }) + + self.assertEqual(500, resp.status_code) + self.assertEqual( + [], self.certbot_deletes(), + 'the lineage must not be deleted when HAProxy did not reload') + self.assertFalse(os.path.exists(old_path)) + quarantined = os.path.join(hm.cert_backup_dir(), + os.path.basename(old_path)) + self.assertTrue(os.path.exists(quarantined), + 'the superseded file must be recoverable by hand') + self.assertTrue(structurally_valid(self.read(quarantined))) + + @FIX_ONLY + @NEEDS_OPENSSL + def test_successful_bundle_still_supersedes_and_deletes(self): + """The cleanup must still do its job on the happy path.""" + old_path = self._setup_supersede(broken=False) + + resp = self.client.post('/api/ssl/bundle', json={ + 'primary': 'test.example.com', + 'sans': ['www.test.example.com'], + }) + + self.assertEqual(200, resp.status_code, resp.get_data(as_text=True)) + self.assertFalse(os.path.exists(old_path), + 'the superseded file must leave the crt directory or ' + 'it keeps shadowing the new bundle') + self.assertEqual(['delete --cert-name www.test.example.com -n'], + self.certbot_deletes()) + self.assertTrue(self.edge_would_start()) + self.assert_only_final_pems_in_certs_dir() + + +class TestBundleValidation(CertPublishTestCase): + """What may and may not be published.""" + + @FIX_ONLY + def test_structure_checks(self): + cases = [ + ('', False, 'empty'), + (' \n', False, 'whitespace only'), + (LEAF_CERT, False, 'certificate without a key'), + (LEAF_KEY, False, 'key without a certificate'), + (GOOD_BUNDLE[:len(LEAF_CERT) // 2], False, 'truncated mid-block'), + (LEAF_CERT + LEAF_KEY.replace('-----END PRIVATE KEY-----', ''), + False, 'key block never closed'), + (GOOD_BUNDLE, True, 'complete bundle'), + ] + for text, expected, label in cases: + with self.subTest(label): + ok, _ = hm.validate_pem_structure(text) + self.assertEqual(expected, ok) + + @FIX_ONLY + @NEEDS_OPENSSL + def test_key_must_match_the_leaf_certificate(self): + dest = os.path.join(self.certs, 'pair.example.com.pem') + cert = self.write(os.path.join(self.tmp, 'src', 'fullchain.pem'), LEAF_CERT) + bad_key = self.write(os.path.join(self.tmp, 'src', 'wrong.pem'), + UNRELATED_KEY) + with self.assertRaises(hm.CertificatePublishError): + hm.publish_pem_bundle(dest, [cert, bad_key]) + self.assertFalse(os.path.exists(dest)) + self.assert_only_final_pems_in_certs_dir() + + @FIX_ONLY + @NEEDS_OPENSSL + def test_mismatched_pair_does_not_disturb_the_live_bundle(self): + """The atomic-swap invariant, not just the pre-write content check. + + A cert+key that only turns out to be unusable once assembled (here: a + structurally perfect bundle whose key belongs to another certificate) + is the case that proves the live file is never opened for writing - + the failure is discovered with the replacement already fully staged. + """ + dest = self.publish_live_bundle('swap.example.com') + before = self.read(dest) + cert = self.write(os.path.join(self.tmp, 'srcm', 'fullchain.pem'), + LEAF_CERT) + bad_key = self.write(os.path.join(self.tmp, 'srcm', 'privkey.pem'), + UNRELATED_KEY) + + with self.assertRaises(hm.CertificatePublishError): + hm.publish_pem_bundle(dest, [cert, bad_key]) + + self.assertEqual(before, self.read(dest), + 'the previously served bundle must survive byte for byte') + self.assertTrue(self.edge_would_start()) + self.assert_only_final_pems_in_certs_dir() + + @FIX_ONLY + def test_unexpected_error_mid_publish_leaves_the_live_bundle_intact(self): + """A failure anywhere between staging and swap must be survivable. + + Stands in for the failures we cannot stage deterministically - disk + full, container killed, an exception in a future validation step. + """ + dest = self.publish_live_bundle('boom.example.com') + before = self.read(dest) + cert = self.write(os.path.join(self.tmp, 'srcb', 'fullchain.pem'), + LEAF_CERT) + key = self.write(os.path.join(self.tmp, 'srcb', 'privkey.pem'), LEAF_KEY) + + real_validate = hm.validate_pem_bundle + + def _explode(path): + raise RuntimeError('simulated failure while publishing') + + hm.validate_pem_bundle = _explode + self.addCleanup(lambda: setattr(hm, 'validate_pem_bundle', real_validate)) + + with self.assertRaises(Exception): + hm.publish_pem_bundle(dest, [cert, key]) + + self.assertEqual(before, self.read(dest)) + self.assertTrue(self.edge_would_start()) + self.assert_only_final_pems_in_certs_dir() + + @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()): + self.assertFalse( + os.path.abspath(path).startswith(os.path.abspath(self.certs) + os.sep), + f'{path} must not be inside {self.certs}') + + @FIX_ONLY + def test_previous_bundle_is_backed_up_on_publish(self): + dest = self.publish_live_bundle('backup.example.com') + previous = self.read(dest) + cert = self.write(os.path.join(self.tmp, 'src2', 'fullchain.pem'), + LEAF_CERT) + key = self.write(os.path.join(self.tmp, 'src2', 'privkey.pem'), LEAF_KEY) + + hm.publish_pem_bundle(dest, [cert, key]) + + backup = os.path.join(hm.cert_backup_dir(), 'backup.example.com.pem') + self.assertTrue(os.path.exists(backup), + 'an operator needs a manual path back to the previous ' + 'certificate') + self.assertEqual(previous, self.read(backup)) + + @FIX_ONLY + def test_corrupt_live_bundle_does_not_overwrite_a_good_backup(self): + """Mirrors create_backup(require_valid=True) for haproxy.cfg.""" + dest = os.path.join(self.certs, 'guard.example.com.pem') + os.makedirs(hm.cert_backup_dir(), exist_ok=True) + good_backup = self.write( + os.path.join(hm.cert_backup_dir(), 'guard.example.com.pem'), + GOOD_BUNDLE) + self.write(dest, LEAF_CERT) # live file is key-less garbage + + cert = self.write(os.path.join(self.tmp, 'src3', 'fullchain.pem'), + LEAF_CERT) + key = self.write(os.path.join(self.tmp, 'src3', 'privkey.pem'), LEAF_KEY) + hm.publish_pem_bundle(dest, [cert, key]) + + self.assertEqual(GOOD_BUNDLE, self.read(good_backup), + 'a good backup must not be replaced by a corrupt live ' + 'file') + + @FIX_ONLY + def test_no_temp_file_survives_a_failed_publish(self): + 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'), '') + with self.assertRaises(hm.CertificatePublishError): + hm.publish_pem_bundle(dest, [cert, empty]) + 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') + + +class TestClusterSecretSelfHeal(CertPublishTestCase): + """Bug 4: a zero-byte secret file was never healed.""" + + def test_empty_secret_file_is_healed(self): + self.write(hm.CLUSTER_SECRET_PATH, '') + secret = hm.get_or_create_cluster_secret() + self.assertTrue( + secret, + 'an empty secret file must be regenerated, not returned as ""') + self.assertEqual(secret, self.read(hm.CLUSTER_SECRET_PATH).strip()) + self.assertEqual(secret, hm.get_or_create_cluster_secret(), + 'the healed secret must then be stable') + + def test_existing_secret_is_preserved(self): + self.write(hm.CLUSTER_SECRET_PATH, 'deadbeef\n') + self.assertEqual('deadbeef', hm.get_or_create_cluster_secret()) + + +if __name__ == '__main__': + print(f"testing haproxy_manager from: {MODULE_DIR}", file=sys.stderr) + unittest.main(verbosity=2) From 4f0949d5341eb6cc096df2a49c185fa2a26aa303 Mon Sep 17 00:00:00 2001 From: Josh Knapp Date: Thu, 6 Aug 2026 10:02:28 -0700 Subject: [PATCH 4/4] 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 '..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) --- haproxy_manager.py | 80 ++++-- scripts/cert-publish-lib.sh | 177 +++++++++---- scripts/renew-certificates.sh | 13 + scripts/sync-certificates.sh | 7 + scripts/test-cert-scripts.py | 406 ++++++++++++++++++++++++++---- scripts/test-cert-write-safety.py | 239 ++++++++++++++++-- 6 files changed, 792 insertions(+), 130 deletions(-) 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):