From 43c391c452d86329e93abeeb48275eea98a53829 Mon Sep 17 00:00:00 2001 From: "shipmill-romamo[bot]" <338557544+shipmill-romamo[bot]@users.noreply.github.com> Date: Tue, 27 Oct 2026 13:17:30 +0200 Subject: [PATCH] Reload an owed pg_hba.conf and refuse keyword names at validate A failed pg_hba.conf check or reload made the rescue remove the Cloudfall block, after which the file no longer changed, so later converges never reloaded and PostgreSQL kept the old hostssl lines live until a restart. The rescue now leaves a per-service marker under /var/lib/cloudfall/postgresql; the next converge checks and reloads when the file changed or the marker exists, and removes the marker once pg_reload_conf() returns t. A run with neither stays unchanged. cloudfall config validate now refuses an owned database or owner named all, sameuser, samerole, samegroup or replication when the service declares remoteAccess (service_remote_access_name_reserved), instead of leaving the refusal to the role's converge-time guard, which stays. Fixes #144 Co-Authored-By: Claude Opus 5.5 --- changelog.d/144-pg-hba-reload-and-names.md | 4 ++ docs/specs/006-standalone-postgresql.md | 8 ++- .../cloudfall_postgresql/defaults/main.yml | 1 + .../meta/argument_specs.yml | 6 ++ .../roles/cloudfall_postgresql/tasks/main.yml | 49 ++++++++++++- engine/tests/test_postgresql_remote_access.py | 70 ++++++++++++++++++- sdk/src/cloudfall/validation.py | 35 ++++++++++ sdk/tests/test_owned_postgresql.py | 36 ++++++++++ 8 files changed, 202 insertions(+), 7 deletions(-) create mode 100644 changelog.d/144-pg-hba-reload-and-names.md diff --git a/changelog.d/144-pg-hba-reload-and-names.md b/changelog.d/144-pg-hba-reload-and-names.md new file mode 100644 index 0000000..00e585a --- /dev/null +++ b/changelog.d/144-pg-hba-reload-and-names.md @@ -0,0 +1,4 @@ +### Fixed + +- A `pg_hba.conf` reload that failed is no longer forgotten: `services.yml` leaves a marker file at `/var/lib/cloudfall/postgresql/.pg_hba-reload-pending` when it closes remote access after a failed check or reload, and the next converge checks and reloads `pg_hba.conf` even though the file is unchanged, then removes the marker, so the old `hostssl` lines no longer stay live until a restart (#144) +- `cloudfall config validate` refuses an owned database or owner named `all`, `sameuser`, `samerole`, `samegroup` or `replication` on a PostgreSQL service with `remoteAccess`, as `CONFIG_INVALID` with code `service_remote_access_name_reserved`, instead of leaving the refusal to the converge; without `remoteAccess` those names still validate (#144) diff --git a/docs/specs/006-standalone-postgresql.md b/docs/specs/006-standalone-postgresql.md index 651e94e..56d8453 100644 --- a/docs/specs/006-standalone-postgresql.md +++ b/docs/specs/006-standalone-postgresql.md @@ -79,8 +79,10 @@ same `bind.port` serves both addresses. With `remoteAccess` declared: on the service names an `owner` (`service_remote_access_unused`), when the server's server type declares no `firewall` (`service_remote_access_unfirewalled`), when that firewall's `allowedInbound` already opens the service port to everyone -(`service_remote_access_port_open`), or when a source is `0.0.0.0/0` or `::/0` -(`service_remote_access_open`). With `remoteAccess` absent, PostgreSQL listens on +(`service_remote_access_port_open`), when a source is `0.0.0.0/0` or `::/0` +(`service_remote_access_open`), or when an `owner` database or its owner is named `all`, +`sameuser`, `samerole`, `samegroup` or `replication`, words `pg_hba.conf` reads as keywords +(`service_remote_access_name_reserved`). Without `remoteAccess` those names stay allowed. With `remoteAccess` absent, PostgreSQL listens on `127.0.0.1` only, `pg_hba.conf` keeps no Cloudfall block (a block left by an earlier declaration is removed), and no PostgreSQL rule is added to nftables. Closed is the default. @@ -118,7 +120,7 @@ Touches `config/schemas/v1/service.schema.json`, `sdk/src/cloudfall/validation.p - S-006-8: With `remoteAccess` of `listenAddress: 10.0.0.2` and `allowedSources: [10.0.0.0/24]`, the converged `cloudfall.conf` reads `listen_addresses = '127.0.0.1,10.0.0.2'` and `ssl = on`, the Cloudfall block in `pg_hba.conf` holds exactly `hostssl billing billing 10.0.0.0/24 scram-sha-256`, and the rendered nftables ruleset holds `ip saddr 10.0.0.0/24 tcp dport 5432 accept` and no unrestricted rule for port 5432 - S-006-9: On a proving pair of hosts, `psql` from an address in `allowedSources` as `billing` with `sslmode=require` connects, from an address outside it gets no connection, and from an allowed address with `sslmode=disable` is refused with `no pg_hba.conf entry` - S-006-10: Removing `remoteAccess` and converging baseline and services leaves `listen_addresses = '127.0.0.1'`, no Cloudfall block in `pg_hba.conf`, and no rule for the service port in nftables -- S-006-11: `remoteAccess` fails `cloudfall config validate` with `CONFIG_INVALID` and code `service_remote_access_unused` when no database names an `owner`, `service_remote_access_unfirewalled` when the server type declares no `firewall`, `service_remote_access_port_open` when its `allowedInbound` opens the service port, and `service_remote_access_open` for a source of `0.0.0.0/0` or `::/0` +- S-006-11: `remoteAccess` fails `cloudfall config validate` with `CONFIG_INVALID` and code `service_remote_access_unused` when no database names an `owner`, `service_remote_access_unfirewalled` when the server type declares no `firewall`, `service_remote_access_port_open` when its `allowedInbound` opens the service port, `service_remote_access_open` for a source of `0.0.0.0/0` or `::/0`, and `service_remote_access_name_reserved` for an `owner` database or owner named `all`, `sameuser`, `samerole`, `samegroup` or `replication` - S-006-12: For the project in `config/examples`, `cloudfall-engine inventory render` writes a byte-identical inventory before and after the change, and the rendered `cloudfall.conf`, `nftables.conf`, backup script, and restore-check script are identical - S-006-13: For the service of S-006-1, the backup script writes `billing-.dump`, the restore drill restores it and passes, and `cloudfall data copy --database billing --source-url-file --yes` restores as role `billing` and writes a receipt with `spec.owner` of `billing` diff --git a/engine/ansible/roles/cloudfall_postgresql/defaults/main.yml b/engine/ansible/roles/cloudfall_postgresql/defaults/main.yml index 9b6c676..be8d5fd 100644 --- a/engine/ansible/roles/cloudfall_postgresql/defaults/main.yml +++ b/engine/ansible/roles/cloudfall_postgresql/defaults/main.yml @@ -9,3 +9,4 @@ cloudfall_postgresql_password_path: /var/lib/postgresql/.cloudfall-owner-passwor cloudfall_postgresql_ssl_cert_file: /etc/ssl/certs/ssl-cert-snakeoil.pem cloudfall_postgresql_ssl_key_file: /etc/ssl/private/ssl-cert-snakeoil.key cloudfall_postgresql_hba_marker: "# {mark} CLOUDFALL MANAGED REMOTE ACCESS" +cloudfall_postgresql_state_directory: /var/lib/cloudfall/postgresql diff --git a/engine/ansible/roles/cloudfall_postgresql/meta/argument_specs.yml b/engine/ansible/roles/cloudfall_postgresql/meta/argument_specs.yml index f0230f5..e71fde9 100644 --- a/engine/ansible/roles/cloudfall_postgresql/meta/argument_specs.yml +++ b/engine/ansible/roles/cloudfall_postgresql/meta/argument_specs.yml @@ -120,6 +120,12 @@ argument_specs: type: str default: "# {mark} CLOUDFALL MANAGED REMOTE ACCESS" description: Markers of the remoteAccess block in pg_hba.conf + cloudfall_postgresql_state_directory: + type: path + default: /var/lib/cloudfall/postgresql + description: >- + Host directory of the role's state, such as a service's pending + pg_hba.conf reload, one file per service id cloudfall_postgresql_metrics_role: type: str default: alloy diff --git a/engine/ansible/roles/cloudfall_postgresql/tasks/main.yml b/engine/ansible/roles/cloudfall_postgresql/tasks/main.yml index b92972f..c2d0b22 100644 --- a/engine/ansible/roles/cloudfall_postgresql/tasks/main.yml +++ b/engine/ansible/roles/cloudfall_postgresql/tasks/main.yml @@ -206,13 +206,24 @@ # The block is written with remoteAccess and removed without it, so an # earlier opening never lingers; lines outside the markers stay untouched. +# A reload that failed leaves a marker file, so the next run reloads even +# though the file on disk no longer changes. - name: Converge the remote access block of pg_hba.conf vars: cloudfall_postgresql_hba_path: >- /etc/postgresql/{{ cloudfall_postgresql_service.postgresql.majorVersion }}/main/pg_hba.conf + cloudfall_postgresql_hba_reload_marker: >- + {{ cloudfall_postgresql_state_directory }}/{{ + cloudfall_postgresql_service.id + }}.pg_hba-reload-pending block: + - name: Read whether an earlier pg_hba.conf reload is pending + ansible.builtin.stat: + path: "{{ cloudfall_postgresql_hba_reload_marker }}" + register: cloudfall_postgresql_hba_pending + - name: Write or remove the Cloudfall block in pg_hba.conf ansible.builtin.blockinfile: path: "{{ cloudfall_postgresql_hba_path }}" @@ -253,7 +264,9 @@ or cloudfall_postgresql_hba_errors.stdout_lines | length > 0 when: - not ansible_check_mode - - cloudfall_postgresql_hba is changed + - >- + cloudfall_postgresql_hba is changed + or cloudfall_postgresql_hba_pending.stat.exists - name: Reload PostgreSQL to apply pg_hba.conf ansible.builtin.command: @@ -274,8 +287,37 @@ or cloudfall_postgresql_hba_reload.stdout | trim != 't' when: - not ansible_check_mode - - cloudfall_postgresql_hba is changed + - >- + cloudfall_postgresql_hba is changed + or cloudfall_postgresql_hba_pending.stat.exists + + - name: Clear the pending pg_hba.conf reload once it applied + ansible.builtin.file: + path: "{{ cloudfall_postgresql_hba_reload_marker }}" + state: absent + when: + - not ansible_check_mode + - cloudfall_postgresql_hba_pending.stat.exists rescue: + # PostgreSQL may still serve the lines of the last reload: record that a + # reload is owed before the file changes again. + - name: Create the PostgreSQL role state directory + ansible.builtin.file: + path: "{{ cloudfall_postgresql_state_directory }}" + state: directory + owner: root + group: root + mode: "0700" + + - name: Record that pg_hba.conf still needs a reload + ansible.builtin.copy: + dest: "{{ cloudfall_postgresql_hba_reload_marker }}" + content: | + {{ cloudfall_postgresql_service.id }}: pg_hba.conf reload pending + owner: root + group: root + mode: "0600" + - name: Close remote access when pg_hba.conf could not be applied ansible.builtin.blockinfile: path: "{{ cloudfall_postgresql_hba_path }}" @@ -286,7 +328,8 @@ ansible.builtin.fail: msg: >- pg_hba.conf of service {{ cloudfall_postgresql_service.id }} was not - reloaded and its Cloudfall block was removed: + reloaded and its Cloudfall block was removed; the next converge + reloads it: {{ cloudfall_postgresql_hba_errors.stdout_lines | default([]) | join('; ') diff --git a/engine/tests/test_postgresql_remote_access.py b/engine/tests/test_postgresql_remote_access.py index fe4d395..b32b18a 100644 --- a/engine/tests/test_postgresql_remote_access.py +++ b/engine/tests/test_postgresql_remote_access.py @@ -438,6 +438,9 @@ def test_a_source_restricted_rule_does_not_count_as_the_ssh_port() -> None: if errors.exists(): print(errors.read_text(), end="") elif "pg_reload_conf" in command: + if log.joinpath("reload-fails").exists(): + print("f") + sys.exit() with log.joinpath("reloads").open("a") as reloads: reloads.write("reload\\n") print("t") @@ -446,6 +449,14 @@ def test_a_source_restricted_rule_does_not_count_as_the_ssh_port() -> None: """ +def _state(tmp_path: Path) -> Path: + return tmp_path / "state" + + +def _pending_reload(tmp_path: Path) -> Path: + return _state(tmp_path) / "postgresql-main.pg_hba-reload-pending" + + def _hba_play( tmp_path: Path, service: dict[str, object], hba: Path, log: Path ) -> Path: @@ -461,11 +472,16 @@ def _hba_play( if task["name"] == "Converge the remote access block of pg_hba.conf" ) block["vars"]["cloudfall_postgresql_hba_path"] = str(hba) - for task in block["block"]: + for task in [*block["block"], *block["rescue"]]: command = task.get("ansible.builtin.command") if command is not None: command["argv"][0] = str(psql) del task["become"], task["become_user"] + # The test runs unprivileged; the host's files stay root's. + for module in ("ansible.builtin.file", "ansible.builtin.copy"): + if module in task: + task[module].pop("owner", None) + task[module].pop("group", None) templates = tmp_path / "templates" templates.mkdir(exist_ok=True) shutil.copy(POSTGRESQL / "templates" / "pg_hba-remote-access.j2", templates) @@ -480,6 +496,7 @@ def _hba_play( "cloudfall_postgresql_hba_marker": _defaults()[ "cloudfall_postgresql_hba_marker" ], + "cloudfall_postgresql_state_directory": str(_state(tmp_path)), }, "tasks": [block], } @@ -584,3 +601,54 @@ def test_a_pg_hba_conf_postgresql_cannot_read_is_never_reloaded( assert _reloads(log) == 0 # The opening is closed again, so it never takes effect at a later restart. assert hba.read_text(encoding="utf-8") == DEBIAN_HBA + + +def test_a_failed_reload_is_owed_until_a_later_converge_applies_it( + tmp_path: Path, +) -> None: + project = _project(tmp_path, REMOTE_ACCESS) + opened = _service(_host(_inventory(project))) + _project(tmp_path, None) + closed = _service(_host(_inventory(project))) + log = tmp_path / "psql-log" + log.mkdir() + hba = tmp_path / "pg_hba.conf" + hba.write_text(DEBIAN_HBA, encoding="utf-8") + opening = _converge_hba(tmp_path, opened, hba, log) + (log / "reload-fails").touch() + + failed = _converge_hba(tmp_path, closed, hba, log) + marker_after_failure = _pending_reload(tmp_path).read_text(encoding="utf-8") + (log / "reload-fails").unlink() + retried = _converge_hba(tmp_path, closed, hba, log) + reloads_after_retry = _reloads(log) + settled = _converge_hba(tmp_path, closed, hba, log) + + assert opening.returncode == 0, opening.stdout + opening.stderr + assert failed.returncode != 0 + assert marker_after_failure == "postgresql-main: pg_hba.conf reload pending\n" + # The file no longer changes, yet the owed reload runs and is cleared. + assert retried.returncode == 0, retried.stdout + retried.stderr + assert reloads_after_retry == 2 + assert not _pending_reload(tmp_path).exists() + # With neither a change nor a marker, nothing reloads or reports a change. + assert settled.returncode == 0, settled.stdout + settled.stderr + assert _reloads(log) == 2 + assert "changed=0" in settled.stdout + assert hba.read_text(encoding="utf-8") == DEBIAN_HBA + + +def test_a_pg_hba_conf_postgresql_cannot_read_owes_a_reload( + tmp_path: Path, +) -> None: + opened = _service(_host(_inventory(_project(tmp_path, REMOTE_ACCESS)))) + log = tmp_path / "psql-log" + log.mkdir() + (log / "errors").write_text('3: invalid connection type "hostsssl"\n') + hba = tmp_path / "pg_hba.conf" + hba.write_text(DEBIAN_HBA, encoding="utf-8") + + done = _converge_hba(tmp_path, opened, hba, log) + + assert done.returncode != 0 + assert _pending_reload(tmp_path).exists() diff --git a/sdk/src/cloudfall/validation.py b/sdk/src/cloudfall/validation.py index 23b0288..8dfd5ed 100644 --- a/sdk/src/cloudfall/validation.py +++ b/sdk/src/cloudfall/validation.py @@ -698,9 +698,44 @@ def _validate_postgresql_service( index, has_owner=bool(owners), ) + _validate_remote_access_names(document, databases) _RESERVED_POSTGRES_ROLES = frozenset({"postgres", "alloy"}) +# pg_hba.conf reads these bare words as keywords, not names: a database named +# all would open every database to its owner, an owner named all every role. +_PG_HBA_KEYWORDS = frozenset( + {"all", "sameuser", "samerole", "samegroup", "replication"} +) + + +def _validate_remote_access_names( + document: ResourceDocument, + databases: Sequence[tuple[int, Mapping[str, object]]], +) -> None: + """Refuse an owned database or owner pg_hba.conf would read as a keyword.""" + for position, database in databases: + if "owner" not in database: + continue + for field_name in ("name", "owner"): + value = database.get(field_name) + if value in _PG_HBA_KEYWORDS: + issue = ValidationIssue( + code="service_remote_access_name_reserved", + message=( + f"database {field_name} {value!r} is a pg_hba.conf " + "keyword; remoteAccess cannot name it in a hostssl line" + ), + source=document.source, + field_path=( + "spec", + "postgresql", + "databases", + position, + field_name, + ), + ) + raise ConfigValidationError(issue) def _validate_service_database_owners( diff --git a/sdk/tests/test_owned_postgresql.py b/sdk/tests/test_owned_postgresql.py index 78e34d6..573c127 100644 --- a/sdk/tests/test_owned_postgresql.py +++ b/sdk/tests/test_owned_postgresql.py @@ -334,3 +334,39 @@ def test_remote_access_listen_address_is_one_canonical_address( "remoteAccess", "listenAddress", ) + + +PG_HBA_KEYWORDS = ["all", "sameuser", "samerole", "samegroup", "replication"] + + +@pytest.mark.parametrize("keyword", PG_HBA_KEYWORDS) +@pytest.mark.parametrize("field_name", ["name", "owner"]) +def test_remote_access_refuses_a_name_pg_hba_reads_as_a_keyword( + tmp_path: Path, + capsys: pytest.CaptureFixture[str], + keyword: str, + field_name: str, +) -> None: + database = _billing(**{field_name: keyword}) + project = _standalone_project( + tmp_path, _owned_service([database], remote_access=REMOTE_ACCESS) + ) + + exit_code, document = _validate(project, capsys) + + assert exit_code == 80 + assert document["error"]["code"] == "CONFIG_INVALID" + context = document["error"]["context"] + assert context["code"] == "service_remote_access_name_reserved" + assert context["field"] == f"spec.postgresql.databases.0.{field_name}" + + +@pytest.mark.parametrize("keyword", PG_HBA_KEYWORDS) +@pytest.mark.parametrize("field_name", ["name", "owner"]) +def test_a_keyword_name_validates_without_remote_access( + tmp_path: Path, keyword: str, field_name: str +) -> None: + database = _billing(**{field_name: keyword}) + project = _standalone_project(tmp_path, _owned_service([database])) + + assert validate_config(project, SCHEMAS).resource_count == 3