Reload an owed pg_hba.conf and refuse pg_hba keyword names at validate (#144)

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/<service>.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)

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-<timestamp>.dump`, the restore drill restores it and passes, and `cloudfall data copy <service> --database billing --source-url-file <file> --yes` restores as role `billing` and writes a receipt with `spec.owner` of `billing`
 

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

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

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('; ')

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()

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(

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