From bc4677bf289ac50d17a3330bd0cee7e0fa0db641 Mon Sep 17 00:00:00 2001 From: "shipmill-romamo[bot]" <338557544+shipmill-romamo[bot]@users.noreply.github.com> Date: Fri, 9 Oct 2026 21:04:13 +0300 Subject: [PATCH] Spell HTTP/2 for the installed Nginx and roll back a rejected vhost The domain vhost wrote `http2 on;`, which Nginx only parses from 1.25.1; Debian 12 ships 1.22.1, so `nginx -t` failed with `unknown directive "http2"`. The role now reads the installed version with `nginx -v` (also in check mode, failing the converge when it cannot be parsed) and writes `listen 443 ssl http2;` below 1.25.1. Output on 1.25.1 and later is unchanged. A rejected render used to stay on disk and enabled, so the next reload (a certbot hook, a reboot) broke every site on the host. Each render, link and `nginx -t` now runs in a block whose rescue restores the previous file (kept in memory with slurp, so no backup file lands where Nginx reads), or removes a new file and a link this run created, then fails with Nginx's error. The rescue ends in a failure, so with force_handlers off (neither the playbook, ansible.cfg nor the engine set it) the queued reload never runs. Nginx is started only after the first configuration test passes. Fixes #66 Co-Authored-By: Claude Opus 5.5 --- CHANGELOG.md | 11 + .../tasks/apply_virtual_host.yml | 89 ++++++++ .../roles/cloudfall_nginx_site/tasks/main.yml | 105 +++++---- .../templates/domain-site.conf.j2 | 5 + engine/tests/test_nginx_site_role.py | 203 ++++++++++++++++++ engine/tests/test_nginx_site_templates.py | 41 +++- 6 files changed, 408 insertions(+), 46 deletions(-) create mode 100644 engine/ansible/roles/cloudfall_nginx_site/tasks/apply_virtual_host.yml create mode 100644 engine/tests/test_nginx_site_role.py diff --git a/CHANGELOG.md b/CHANGELOG.md index 829b34e..b5c4d03 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -534,6 +534,17 @@ Notable changes to Cloudfall. The format follows `spec.steps[].arguments` only holds arguments that ran whole; the trace still keeps the start of the raw arguments. `agent-investigation.schema.json` bounds both finding fields with `maxLength` (#63) +- `domains` deploys a TLS route on Debian 12. The vhost wrote `http2 on;`, + which Nginx only knows from 1.25.1, so on Debian's 1.22.1 `nginx -t` + failed with `unknown directive "http2"`. The role now reads the installed + version with `nginx -v` (failing the converge when it cannot) and writes + `listen 443 ssl http2;` below 1.25.1; the rendered vhost on 1.25.1 and + later is unchanged. A vhost that `nginx -t` rejects no longer stays on + disk and enabled for the next reload (a certbot hook, a reboot) to take + every site on the host down: the role restores the previous file, or + removes a new file and the `sites-enabled` link this run created, then + fails with Nginx's error and without reloading. Nginx is now started only + after the configuration test passes (#66) ### Security diff --git a/engine/ansible/roles/cloudfall_nginx_site/tasks/apply_virtual_host.yml b/engine/ansible/roles/cloudfall_nginx_site/tasks/apply_virtual_host.yml new file mode 100644 index 0000000..fd187eb --- /dev/null +++ b/engine/ansible/roles/cloudfall_nginx_site/tasks/apply_virtual_host.yml @@ -0,0 +1,89 @@ +--- +# A render goes live only behind `nginx -t`. What was on disk before is +# recorded first, so a rejected render is rolled back before the play fails: +# otherwise the invalid vhost stays enabled and the next reload (a certbot +# hook, a reboot) takes every site on the host down with it. The rescue ends +# in a failure, so with force_handlers off the queued reload never runs. +- name: Inspect the current virtual host + ansible.builtin.stat: + path: "{{ cloudfall_nginx_site_domain.proxy.configurationPath }}" + get_checksum: false + register: cloudfall_nginx_site_previous_file + +- name: Keep the current virtual host for rollback + ansible.builtin.slurp: + src: "{{ cloudfall_nginx_site_domain.proxy.configurationPath }}" + register: cloudfall_nginx_site_previous_content + when: cloudfall_nginx_site_previous_file.stat.exists + +- name: Inspect the current virtual host link + ansible.builtin.stat: + path: "{{ cloudfall_nginx_site_link }}" + get_checksum: false + register: cloudfall_nginx_site_previous_link + when: cloudfall_nginx_site_link | length > 0 + +- name: Apply the virtual host behind the configuration test + block: + - name: Render the managed virtual host + ansible.builtin.template: + src: domain-site.conf.j2 + dest: "{{ cloudfall_nginx_site_domain.proxy.configurationPath }}" + owner: root + group: root + mode: "0644" + notify: Reload Nginx + + - name: Enable the managed virtual host + ansible.builtin.file: + src: "{{ cloudfall_nginx_site_domain.proxy.configurationPath }}" + dest: "{{ cloudfall_nginx_site_link }}" + state: link + notify: Reload Nginx + when: cloudfall_nginx_site_link | length > 0 + + - name: Validate the Nginx configuration + ansible.builtin.command: + argv: + - /usr/sbin/nginx + - -t + changed_when: false + when: not ansible_check_mode + rescue: + - name: Restore the previous virtual host + ansible.builtin.copy: + content: >- + {{ cloudfall_nginx_site_previous_content.content | b64decode }} + dest: "{{ cloudfall_nginx_site_domain.proxy.configurationPath }}" + owner: "{{ cloudfall_nginx_site_previous_file.stat.pw_name }}" + group: "{{ cloudfall_nginx_site_previous_file.stat.gr_name }}" + mode: "{{ cloudfall_nginx_site_previous_file.stat.mode }}" + when: + - not ansible_check_mode + - cloudfall_nginx_site_previous_file.stat.exists + + - name: Remove the rejected new virtual host + ansible.builtin.file: + path: "{{ cloudfall_nginx_site_domain.proxy.configurationPath }}" + state: absent + when: + - not ansible_check_mode + - not cloudfall_nginx_site_previous_file.stat.exists + + - name: Remove the link this run created + ansible.builtin.file: + path: "{{ cloudfall_nginx_site_link }}" + state: absent + when: + - not ansible_check_mode + - cloudfall_nginx_site_link | length > 0 + - not cloudfall_nginx_site_previous_link.stat.exists + + - name: Fail after rolling back the rejected virtual host + ansible.builtin.fail: + msg: >- + {{ cloudfall_nginx_site_domain.proxy.configurationPath }} was + rejected and rolled back, and Nginx was not reloaded: {{ + ansible_failed_result.stderr | default('', true) + or ansible_failed_result.msg | default('') + }} diff --git a/engine/ansible/roles/cloudfall_nginx_site/tasks/main.yml b/engine/ansible/roles/cloudfall_nginx_site/tasks/main.yml index c271812..01136cf 100644 --- a/engine/ansible/roles/cloudfall_nginx_site/tasks/main.yml +++ b/engine/ansible/roles/cloudfall_nginx_site/tasks/main.yml @@ -20,6 +20,47 @@ update_cache: true cache_valid_time: 3600 +# The TLS server block spells HTTP/2 the way the installed Nginx parses it: +# `http2 on;` exists only from 1.25.1, and Debian 12 ships 1.22.1. Nginx +# prints its version on stderr. The probe also runs in check mode, since the +# template needs the version there too. +- name: Read the installed Nginx version + ansible.builtin.command: + argv: + - /usr/sbin/nginx + - -v + register: cloudfall_nginx_site_version_probe + changed_when: false + failed_when: false + check_mode: false + +- name: Require a readable Nginx version + ansible.builtin.assert: + that: + - cloudfall_nginx_site_version_probe.rc == 0 + - >- + cloudfall_nginx_site_version_probe.stderr is search( + '^nginx version: nginx/[0-9]+[.][0-9]+[.][0-9]+', multiline=True + ) + fail_msg: >- + Cannot read the installed Nginx version from `nginx -v` (exit {{ + cloudfall_nginx_site_version_probe.rc + }}): {{ + cloudfall_nginx_site_version_probe.stderr | default('', true) + or cloudfall_nginx_site_version_probe.msg | default('') + }} + +- name: Record the installed Nginx version + ansible.builtin.set_fact: + cloudfall_nginx_site_version: >- + {{ + cloudfall_nginx_site_version_probe.stderr + | regex_findall( + '^nginx version: nginx/([0-9]+[.][0-9]+[.][0-9]+)', multiline=True + ) + | first + }} + - name: Create the ACME challenge webroot ansible.builtin.file: path: "{{ cloudfall_nginx_site_acme_webroot }}" @@ -50,43 +91,31 @@ and cloudfall_nginx_site_certificate.stat.exists }} -- name: Render the managed virtual host - ansible.builtin.template: - src: domain-site.conf.j2 - dest: "{{ cloudfall_nginx_site_domain.proxy.configurationPath }}" - owner: root - group: root - mode: "0644" - notify: Reload Nginx - -- name: Enable the managed virtual host - ansible.builtin.file: - src: "{{ cloudfall_nginx_site_domain.proxy.configurationPath }}" - dest: >- - /etc/nginx/sites-enabled/{{ - cloudfall_nginx_site_domain.proxy.configurationPath | basename +# Only a vhost under sites-available is linked into sites-enabled; an empty +# link path means the declared path is read by Nginx where it is. +- name: Locate the managed virtual host link + ansible.builtin.set_fact: + cloudfall_nginx_site_link: >- + {{ + '/etc/nginx/sites-enabled/' + ~ (cloudfall_nginx_site_domain.proxy.configurationPath | basename) + if cloudfall_nginx_site_domain.proxy.configurationPath.startswith( + '/etc/nginx/sites-available/' + ) + else '' }} - state: link - notify: Reload Nginx - when: >- - cloudfall_nginx_site_domain.proxy.configurationPath.startswith( - '/etc/nginx/sites-available/' - ) +- name: Apply the virtual host before issuance + ansible.builtin.include_tasks: apply_virtual_host.yml + +# Nginx is started only after the configuration test passed, so a rejected +# vhost never reaches a starting server. - name: Enable Nginx ansible.builtin.systemd_service: name: nginx.service enabled: true state: started -- name: Validate the Nginx configuration before issuance - ansible.builtin.command: - argv: - - /usr/sbin/nginx - - -t - changed_when: false - when: not ansible_check_mode - - name: Serve the ACME challenge location before issuance ansible.builtin.meta: flush_handlers @@ -133,14 +162,8 @@ and cloudfall_nginx_site_certificate.stat.exists }} -- name: Render the managed virtual host with issued material - ansible.builtin.template: - src: domain-site.conf.j2 - dest: "{{ cloudfall_nginx_site_domain.proxy.configurationPath }}" - owner: root - group: root - mode: "0644" - notify: Reload Nginx +- name: Apply the virtual host with issued material + ansible.builtin.include_tasks: apply_virtual_host.yml - name: Enable the certificate renewal timer ansible.builtin.systemd_service: @@ -149,14 +172,6 @@ state: started when: cloudfall_nginx_site_domain.tls.mode == "required" -- name: Validate the complete Nginx configuration - ansible.builtin.command: - argv: - - /usr/sbin/nginx - - -t - changed_when: false - when: not ansible_check_mode - - name: Apply the validated virtual host ansible.builtin.meta: flush_handlers diff --git a/engine/ansible/roles/cloudfall_nginx_site/templates/domain-site.conf.j2 b/engine/ansible/roles/cloudfall_nginx_site/templates/domain-site.conf.j2 index 5bce8a3..6cb8c58 100644 --- a/engine/ansible/roles/cloudfall_nginx_site/templates/domain-site.conf.j2 +++ b/engine/ansible/roles/cloudfall_nginx_site/templates/domain-site.conf.j2 @@ -39,9 +39,14 @@ server { {% if cloudfall_nginx_site_tls_active %} server { +{% if cloudfall_nginx_site_version is version('1.25.1', '>=') %} listen 443 ssl; listen [::]:443 ssl; http2 on; +{% else %} + listen 443 ssl http2; + listen [::]:443 ssl http2; +{% endif %} server_name {{ names }}; ssl_certificate /etc/letsencrypt/live/{{ cloudfall_nginx_site_domain.primaryName }}/fullchain.pem; diff --git a/engine/tests/test_nginx_site_role.py b/engine/tests/test_nginx_site_role.py new file mode 100644 index 0000000..7c06b22 --- /dev/null +++ b/engine/tests/test_nginx_site_role.py @@ -0,0 +1,203 @@ +"""The Nginx site role's decisions, evaluated with Ansible's own templating. + +The role needs a Debian host with Nginx, so these tests replay what it +decides rather than running it: the version it reads from ``nginx -v``, and +which rollback steps run when ``nginx -t`` rejects a render. +""" + +from __future__ import annotations + +from pathlib import Path + +import pytest +from ansible.parsing.dataloader import DataLoader +from ansible.template import Templar, trust_as_template + +ROOT = Path(__file__).parents[2] +TASKS = ROOT / "engine" / "ansible" / "roles" / "cloudfall_nginx_site" / "tasks" +RESTORE = "Restore the previous virtual host" +REMOVE_FILE = "Remove the rejected new virtual host" +REMOVE_LINK = "Remove the link this run created" +FAIL = "Fail after rolling back the rejected virtual host" +SITES_AVAILABLE = "/etc/nginx/sites-available/crm.example.test.conf" +SITES_ENABLED = "/etc/nginx/sites-enabled/crm.example.test.conf" + + +def _tasks(name: str) -> list[dict[str, object]]: + tasks = DataLoader().load_from_file(str(TASKS / name)) + assert isinstance(tasks, list) + return tasks + + +def _task(name: str, tasks: list[dict[str, object]]) -> dict[str, object]: + return next(t for t in tasks if t["name"] == name) + + +def _templar(variables: dict[str, object]) -> Templar: + return Templar(loader=DataLoader(), variables=variables) + + +def _holds(expression: object, variables: dict[str, object]) -> bool: + return bool( + _templar(variables).evaluate_conditional(trust_as_template(str(expression))) + ) + + +def _conditions(task: dict[str, object]) -> list[object]: + value = task.get("when", []) + return value if isinstance(value, list) else [value] + + +def _probe(stderr: str, rc: int = 0) -> dict[str, object]: + return {"cloudfall_nginx_site_version_probe": {"rc": rc, "stderr": stderr}} + + +def _version_readable(probe: dict[str, object]) -> bool: + task = _task("Require a readable Nginx version", _tasks("main.yml")) + assertion = task["ansible.builtin.assert"] + assert isinstance(assertion, dict) + return all(_holds(c, probe) for c in assertion["that"]) + + +def _recorded_version(probe: dict[str, object]) -> object: + task = _task("Record the installed Nginx version", _tasks("main.yml")) + facts = task["ansible.builtin.set_fact"] + assert isinstance(facts, dict) + expression = facts["cloudfall_nginx_site_version"] + return _templar(probe).template(trust_as_template(expression)) + + +@pytest.mark.parametrize( + ("stderr", "version"), + [ + ("nginx version: nginx/1.22.1\n", "1.22.1"), + ("nginx version: nginx/1.24.0 (Ubuntu)\n", "1.24.0"), + ("nginx version: nginx/1.26.3\n", "1.26.3"), + ], +) +def test_the_installed_version_is_read_from_nginx_v( + stderr: str, version: str +) -> None: + probe = _probe(stderr) + + assert _version_readable(probe) + assert _recorded_version(probe) == version + + +@pytest.mark.parametrize( + ("stderr", "rc"), + [ + ("", 2), + ("nginx: [emerg] something else\n", 1), + ("nginx version: openresty/1.25.3.1\n", 0), + ("nginx version: nginx/1.25\n", 0), + ], +) +def test_an_unreadable_version_fails_the_converge(stderr: str, rc: int) -> None: + assert not _version_readable(_probe(stderr, rc)) + + +def _rescue_tasks() -> list[dict[str, object]]: + block = _task( + "Apply the virtual host behind the configuration test", + _tasks("apply_virtual_host.yml"), + ) + rescue = block["rescue"] + assert isinstance(rescue, list) + return rescue + + +def _rollback( + *, + file_existed: bool, + link_existed: bool, + link: str = SITES_ENABLED, + check_mode: bool = False, +) -> set[str]: + """Name every rescue step whose ``when`` holds for this host.""" + variables: dict[str, object] = { + "ansible_check_mode": check_mode, + "cloudfall_nginx_site_link": link, + "cloudfall_nginx_site_previous_file": {"stat": {"exists": file_existed}}, + # A skipped stat registers no `stat`: the link condition guards it. + "cloudfall_nginx_site_previous_link": ( + {"stat": {"exists": link_existed}} if link else {"skipped": True} + ), + } + return { + str(task["name"]) + for task in _rescue_tasks() + if all(_holds(c, variables) for c in _conditions(task)) + } + + +def test_a_rejected_first_render_removes_the_file_and_its_new_link() -> None: + assert _rollback(file_existed=False, link_existed=False) == { + REMOVE_FILE, + REMOVE_LINK, + FAIL, + } + + +def test_a_rejected_rerender_restores_the_file_and_keeps_its_link() -> None: + assert _rollback(file_existed=True, link_existed=True) == {RESTORE, FAIL} + + +def test_a_link_this_run_created_is_removed_beside_a_restored_file() -> None: + assert _rollback(file_existed=True, link_existed=False) == { + RESTORE, + REMOVE_LINK, + FAIL, + } + + +def test_a_vhost_outside_sites_available_has_no_link_to_roll_back() -> None: + assert _rollback(file_existed=False, link_existed=False, link="") == { + REMOVE_FILE, + FAIL, + } + + +@pytest.mark.parametrize("file_existed", [True, False]) +def test_check_mode_rolls_nothing_back_and_still_fails(*, file_existed: bool) -> None: + """Check mode changed nothing on disk, so there is nothing to restore.""" + assert _rollback( + file_existed=file_existed, link_existed=False, check_mode=True + ) == {FAIL} + + +def test_the_rollback_failure_carries_the_nginx_error() -> None: + task = _task(FAIL, _rescue_tasks()) + failure = task["ansible.builtin.fail"] + assert isinstance(failure, dict) + error = 'nginx: [emerg] unknown directive "http2" in crm.example.test.conf:41' + variables = { + "cloudfall_nginx_site_domain": { + "proxy": {"configurationPath": SITES_AVAILABLE} + }, + "ansible_failed_result": {"rc": 1, "stderr": error, "msg": "non-zero"}, + } + + message = _templar(variables).template(trust_as_template(failure["msg"])) + + assert isinstance(message, str) + assert message.startswith(SITES_AVAILABLE) + assert "Nginx was not reloaded" in message + assert message.endswith(error) + + +def test_the_link_is_named_after_a_sites_available_vhost() -> None: + task = _task("Locate the managed virtual host link", _tasks("main.yml")) + facts = task["ansible.builtin.set_fact"] + assert isinstance(facts, dict) + + def link(path: str) -> object: + variables: dict[str, object] = { + "cloudfall_nginx_site_domain": {"proxy": {"configurationPath": path}} + } + return _templar(variables).template( + trust_as_template(facts["cloudfall_nginx_site_link"]) + ) + + assert link(SITES_AVAILABLE) == SITES_ENABLED + assert link("/etc/nginx/conf.d/crm.example.test.conf") == "" diff --git a/engine/tests/test_nginx_site_templates.py b/engine/tests/test_nginx_site_templates.py index 35e888c..7736e92 100644 --- a/engine/tests/test_nginx_site_templates.py +++ b/engine/tests/test_nginx_site_templates.py @@ -4,12 +4,18 @@ from pathlib import Path +import pytest + +# Aliased so pytest does not collect Ansible's plugin class as a test class. +from ansible.plugins.test.core import TestModule as AnsibleTests from jinja2 import Environment, FileSystemLoader, StrictUndefined ROOT = Path(__file__).parents[2] TEMPLATES = ( ROOT / "engine" / "ansible" / "roles" / "cloudfall_nginx_site" / "templates" ) +DEBIAN_12_NGINX = "1.22.1" +FIRST_HTTP2_DIRECTIVE_NGINX = "1.25.1" def _domain() -> dict[str, object]: @@ -29,17 +35,22 @@ def _domain() -> dict[str, object]: } -def _render(*, tls_active: bool) -> str: +def _render(*, tls_active: bool, nginx_version: str = "1.26.3") -> str: + # trim_blocks matches the template module's default. environment = Environment( loader=FileSystemLoader(TEMPLATES), undefined=StrictUndefined, autoescape=False, # noqa: S701 - configuration files are not HTML. keep_trailing_newline=True, + trim_blocks=True, ) + # Ansible's own tests, so `is version(...)` compares as on the host. + environment.tests.update(AnsibleTests().tests()) return environment.get_template("domain-site.conf.j2").render( cloudfall_nginx_site_domain=_domain(), cloudfall_nginx_site_acme_webroot="/var/lib/letsencrypt", cloudfall_nginx_site_tls_active=tls_active, + cloudfall_nginx_site_version=nginx_version, ) @@ -74,3 +85,31 @@ def test_bootstrap_site_serves_http_until_a_certificate_exists() -> None: assert "return 301" not in rendered assert "proxy_pass http://127.0.0.1:8100;" in rendered assert "location /.well-known/acme-challenge/" in rendered + + +@pytest.mark.parametrize("nginx_version", [DEBIAN_12_NGINX, "1.24.0", "1.25.0"]) +def test_nginx_before_1_25_1_gets_http2_on_the_listen_directive( + nginx_version: str, +) -> None: + """`http2 on;` is an unknown directive before 1.25.1 and fails `nginx -t`.""" + rendered = _render(tls_active=True, nginx_version=nginx_version) + + assert " listen 443 ssl http2;\n" in rendered + assert " listen [::]:443 ssl http2;\n" in rendered + assert "http2 on;" not in rendered + + +@pytest.mark.parametrize( + "nginx_version", [FIRST_HTTP2_DIRECTIVE_NGINX, "1.26.3", "1.27.10"] +) +def test_nginx_from_1_25_1_gets_the_http2_directive(nginx_version: str) -> None: + rendered = _render(tls_active=True, nginx_version=nginx_version) + + assert ( + "server {\n" + " listen 443 ssl;\n" + " listen [::]:443 ssl;\n" + " http2 on;\n" + " server_name crm.example.test www.crm.example.test;\n" + ) in rendered + assert "ssl http2;" not in rendered