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