diff --git a/changelog.d/143-audit-remote-access.md b/changelog.d/143-audit-remote-access.md new file mode 100644 index 0000000..8b1c42e --- /dev/null +++ b/changelog.d/143-audit-remote-access.md @@ -0,0 +1,3 @@ +### Fixed + +- `cloudfall audit` no longer reports drift on a server whose PostgreSQL service declares `spec.postgresql.remoteAccess`: `firewall.allowedInbound` expects one `tcp/ from ` entry per declared source and reads the `ip saddr`/`ip6 saddr` match of each accepted nftables rule (a single address printed bare by nft compares equal to its `/32` or `/128`), and `services.bind[]` requires a listener on the declared `listenAddress` as well as on the bind address, reporting `loopbackOnly: false` and a `remoteAccess.listenAddress` key for such a service; a missing `listenAddress` listener, any other non-loopback listener, an unrestricted rule on the port and an undeclared source are drift. Servers without `remoteAccess` audit exactly as before. The engine and the audit now derive the source-restricted rules from one SDK function, `remote_access_rules_for_server` (#143) diff --git a/engine/src/cloudfall_engine/ansible_inventory.py b/engine/src/cloudfall_engine/ansible_inventory.py index 4d0ce78..39f5531 100644 --- a/engine/src/cloudfall_engine/ansible_inventory.py +++ b/engine/src/cloudfall_engine/ansible_inventory.py @@ -4,8 +4,10 @@ from typing import TYPE_CHECKING -from cloudfall.domain import NetworkProtocol -from cloudfall.inventory import firewall_rules_for_server +from cloudfall.inventory import ( + firewall_rules_for_server, + remote_access_rules_for_server, +) if TYPE_CHECKING: from collections.abc import Iterable @@ -172,7 +174,7 @@ def _host_firewall( server_type: ServerTypeInventory, services: tuple[ServiceInventory, ...], ) -> dict[str, object]: - remote_access = _remote_access_rules(server, services) + remote_access = remote_access_rules_for_server(server, services) if server_type.firewall is None: if remote_access: message = ( @@ -187,44 +189,12 @@ def _host_firewall( "policy": server_type.firewall.policy.value, "allowedInbound": [ *(rule.as_dict() for rule in rules), - *remote_access, + *(rule.as_dict() for rule in remote_access), ], } } -def _remote_access_rules( - server: ServerInventory, services: tuple[ServiceInventory, ...] -) -> list[dict[str, object]]: - """Return one source-restricted inbound entry per remotely reachable service. - - The entry carries ``sources``, so the firewall opens the service port to - the declared networks only; a service without ``remoteAccess`` adds none. - """ - rules: list[dict[str, object]] = [] - for service in services: - if service.server_id != server.resource_id or service.postgresql is None: - continue - remote_access = service.postgresql.remote_access - if remote_access is None: - continue - if any(source.is_unrestricted for source in remote_access.allowed_sources): - message = ( - "PostgreSQL remoteAccess refuses a source of every address: " - f"{service.resource_id.value}" - ) - raise ValueError(message) - rules.append( - { - "port": service.bind.port.value, - "protocol": NetworkProtocol.TCP.value, - "sources": [source.value for source in remote_access.allowed_sources], - "description": f"{service.resource_id.value} remoteAccess", - } - ) - return rules - - def _host_logging( server: ServerInventory, logging_stacks: tuple[LoggingStackInventory, ...], diff --git a/sdk/src/cloudfall/audit.py b/sdk/src/cloudfall/audit.py index b1b7a57..1cee7b3 100644 --- a/sdk/src/cloudfall/audit.py +++ b/sdk/src/cloudfall/audit.py @@ -7,9 +7,13 @@ from collections import Counter from dataclasses import dataclass from enum import StrEnum +from ipaddress import ip_network from typing import TYPE_CHECKING, cast -from cloudfall.inventory import firewall_rules_for_server +from cloudfall.inventory import ( + firewall_rules_for_server, + remote_access_rules_for_server, +) if TYPE_CHECKING: from collections.abc import Iterable, Mapping @@ -34,6 +38,8 @@ _MANAGED_FIREWALL_INPUT_CHAIN = "input" _MANAGED_FIREWALL_INPUT_POLICY = "drop" _LOOPBACK_ADDRESSES = frozenset({"127.0.0.1", "::1"}) +# An inbound rule without a saddr match accepts every source. +_ANY_SOURCE = "" _MINIMUM_SOCKET_FIELDS = 5 @@ -209,7 +215,9 @@ def _audit_server( # noqa: PLR0913 - one audit, many evidence sources. checks.extend(_audit_packages(server_type, snapshot)) checks.extend(_audit_services(server_type, snapshot)) if server_type.firewall is not None: - checks.extend(_audit_firewall(server, server_type.firewall, snapshot)) + checks.extend( + _audit_firewall(server, server_type.firewall, services, snapshot) + ) checks.extend(_audit_service_binds(services, snapshot)) checks.extend(_audit_alert_rules(alert_rules, snapshot)) checks.extend( @@ -431,12 +439,13 @@ class _ObservedFirewallTable: """Managed nftables table facts parsed from observation evidence.""" input_policy: str | None - allowed_inbound: tuple[tuple[str, int], ...] + allowed_inbound: tuple[tuple[str, int, str], ...] def _audit_firewall( server: ServerInventory, firewall: FirewallRequirement, + services: tuple[ServiceInventory, ...], snapshot: ObservedServerSnapshot, ) -> tuple[AuditCheck, ...]: evidence = _mapping(snapshot.spec, "firewall") @@ -448,8 +457,15 @@ def _audit_firewall( else None ) expected_rules = sorted( - (rule.protocol.value, rule.port.value) - for rule in firewall_rules_for_server(firewall, server.ssh_port) + [ + (rule.protocol.value, rule.port.value, _ANY_SOURCE) + for rule in firewall_rules_for_server(firewall, server.ssh_port) + ] + + [ + (rule.protocol.value, rule.port.value, source.value) + for rule in remote_access_rules_for_server(server, services) + for source in rule.sources + ] ) observed_rules = None if table is None else sorted(table.allowed_inbound) return ( @@ -470,17 +486,25 @@ def _audit_firewall( ), _comparison( "firewall.allowedInbound", - [f"{protocol}/{port}" for protocol, port in expected_rules], + [_inbound_label(rule) for rule in expected_rules], ( None if observed_rules is None - else [f"{protocol}/{port}" for protocol, port in observed_rules] + else [_inbound_label(rule) for rule in observed_rules] ), matches=observed_rules == expected_rules, ), ) +def _inbound_label(rule: tuple[str, int, str]) -> str: + """Render ``tcp/22``, or ``tcp/5432 from 10.0.0.0/24`` for a source rule.""" + protocol, port, source = rule + if source == _ANY_SOURCE: + return f"{protocol}/{port}" + return f"{protocol}/{port} from {source}" + + def _parse_managed_firewall_table(raw: str) -> _ObservedFirewallTable | None: try: parsed: object = json.loads(raw) @@ -491,7 +515,7 @@ def _parse_managed_firewall_table(raw: str) -> _ObservedFirewallTable | None: return None entries = _mapping_values_sequence(document.get("nftables")) input_policy: str | None = None - allowed: list[tuple[str, int]] = [] + allowed: list[tuple[str, int, str]] = [] table_present = False for entry in entries: table = _optional_mapping(entry.get("table")) @@ -524,9 +548,10 @@ def _parse_managed_firewall_table(raw: str) -> _ObservedFirewallTable | None: def _accepted_inbound_endpoint( rule: Mapping[str, object], -) -> tuple[str, int] | None: +) -> tuple[str, int, str] | None: expressions = _mapping_values_sequence(rule.get("expr")) endpoint: tuple[str, int] | None = None + source = _ANY_SOURCE accepts = False for expression in expressions: if "accept" in expression: @@ -537,18 +562,57 @@ def _accepted_inbound_endpoint( continue left = _optional_mapping(match.get("left")) payload = None if left is None else _optional_mapping(left.get("payload")) + if payload is None: + continue port = match.get("right") if ( - payload is not None - and payload.get("field") == "dport" + payload.get("field") == "dport" and payload.get("protocol") in ("tcp", "udp") and isinstance(port, int) and not isinstance(port, bool) ): endpoint = (cast("str", payload.get("protocol")), port) - if not accepts: + elif payload.get("field") == "saddr" and payload.get("protocol") in ( + "ip", + "ip6", + ): + source = _observed_source(match) + if not accepts or endpoint is None: return None - return endpoint + return (*endpoint, source) + + +def _observed_source(match: Mapping[str, object]) -> str: + """Return a saddr match as a canonical network, as the declaration holds it. + + nftables prints a single address (a /32 or /128) bare and a wider network + as ``{"prefix": {"addr": ..., "len": ...}}``. Any other shape (a set, a + negation, a range) is kept as its raw JSON so it can never match a + declared source and shows up as drift. + """ + right = match.get("right") + prefix = _optional_mapping(right) + inner = None if prefix is None else _optional_mapping(prefix.get("prefix")) + candidate: str | None = None + if isinstance(right, str): + candidate = right + elif inner is not None: + address = inner.get("addr") + length = inner.get("len") + if ( + isinstance(address, str) + and isinstance(length, int) + and not isinstance(length, bool) + ): + candidate = f"{address}/{length}" + raw = json.dumps(dict(match), sort_keys=True) + if match.get("op") != "==" or candidate is None: + return raw + try: + network = ip_network(candidate, strict=True) + except ValueError: + return raw + return str(network) @dataclass(frozen=True, slots=True) @@ -580,11 +644,37 @@ def _audit_service_binds( declared_present = any( listener.address == address for listener in on_port ) + remote_access = ( + None if service.postgresql is None else service.postgresql.remote_access + ) + # The remoteAccess listener is narrowed to its declared sources by the + # firewall, which firewall.allowedInbound verifies; any other + # non-loopback listener is exposed. + accepted = ( + _LOOPBACK_ADDRESSES + if remote_access is None + else _LOOPBACK_ADDRESSES | {remote_access.listen_address.value} + ) exposed = tuple( - listener - for listener in on_port - if listener.address not in _LOOPBACK_ADDRESSES + listener for listener in on_port if listener.address not in accepted ) + # A remoteAccess service must also listen on its listenAddress: one + # that failed to bind it has remote access down while it is declared. + if remote_access is not None: + declared_present = declared_present and any( + listener.address == remote_access.listen_address.value + for listener in on_port + ) + desired: dict[str, object] = { + "protocol": "tcp", + "address": address, + "port": port, + "loopbackOnly": remote_access is None, + } + if remote_access is not None: + desired["remoteAccess"] = { + "listenAddress": remote_access.listen_address.value + } observed = ( { "listeners": [ @@ -598,12 +688,7 @@ def _audit_service_binds( checks.append( _comparison( f"services.bind[{service.resource_id.value}]", - { - "protocol": "tcp", - "address": address, - "port": port, - "loopbackOnly": True, - }, + desired, observed, matches=declared_present and not exposed, ) diff --git a/sdk/src/cloudfall/inventory.py b/sdk/src/cloudfall/inventory.py index 85c7791..038680e 100644 --- a/sdk/src/cloudfall/inventory.py +++ b/sdk/src/cloudfall/inventory.py @@ -2281,6 +2281,57 @@ def firewall_rules_for_server( return (ssh_rule, *firewall.allowed_inbound) +@dataclass(frozen=True, slots=True) +class SourceRestrictedFirewallRule: + """One inbound endpoint opened to declared source networks only.""" + + port: TcpPort + protocol: NetworkProtocol + sources: tuple[IpNetworkCidr, ...] + description: str + + def as_dict(self) -> dict[str, object]: + """Serialize the rule for engine and audit consumers.""" + return { + "port": self.port.value, + "protocol": self.protocol.value, + "sources": [source.value for source in self.sources], + "description": self.description, + } + + +def remote_access_rules_for_server( + server: ServerInventory, services: tuple[ServiceInventory, ...] +) -> tuple[SourceRestrictedFirewallRule, ...]: + """Return one source-restricted inbound rule per remotely reachable service. + + The rule carries the declared sources, so the firewall opens the service + port to those networks only; a service without ``remoteAccess`` adds none. + """ + rules: list[SourceRestrictedFirewallRule] = [] + for service in services: + if service.server_id != server.resource_id or service.postgresql is None: + continue + remote_access = service.postgresql.remote_access + if remote_access is None: + continue + if any(source.is_unrestricted for source in remote_access.allowed_sources): + message = ( + "PostgreSQL remoteAccess refuses a source of every address: " + f"{service.resource_id.value}" + ) + raise ValueError(message) + rules.append( + SourceRestrictedFirewallRule( + port=service.bind.port, + protocol=NetworkProtocol.TCP, + sources=remote_access.allowed_sources, + description=f"{service.resource_id.value} remoteAccess", + ) + ) + return tuple(rules) + + def _configuration_requirement( content: Mapping[str, object], ) -> ConfigurationFileRequirement: diff --git a/sdk/tests/test_audit.py b/sdk/tests/test_audit.py index fdad182..3ef113a 100644 --- a/sdk/tests/test_audit.py +++ b/sdk/tests/test_audit.py @@ -7,7 +7,8 @@ from pathlib import Path import pytest -from cloudfall.audit import AuditReport, AuditStatus, audit_inventory +import yaml +from cloudfall.audit import AuditCheck, AuditReport, AuditStatus, audit_inventory from cloudfall.cli import main from cloudfall.inventory import PlatformInventory from cloudfall.observation import load_observations @@ -390,6 +391,231 @@ def test_audit_reports_missing_service_listener_as_drift( assert bind_check.observed is None +REMOTE_SOURCES = ["10.0.0.0/24", "10.1.0.7/32", "2001:db8::/64"] +REMOTE_LISTENER = "tcp LISTEN 0 244 10.0.0.2:5432 0.0.0.0:*" + + +def _remote_access_project(tmp_path: Path) -> Path: + """Copy the examples and open postgresql-main to declared sources.""" + project = tmp_path / "project" + shutil.copytree(EXAMPLES, project) + path = project / "services" / "postgresql-main.yaml" + service = yaml.safe_load(path.read_text(encoding="utf-8")) + postgresql = service["spec"]["postgresql"] + postgresql["databases"].append( + { + "name": "billing", + "owner": "billing", + "passwordRef": { + "environment": "production", + "path": "/billing/postgresql", + "key": "BILLING_PASSWORD", + }, + } + ) + postgresql["remoteAccess"] = { + "listenAddress": "10.0.0.2", + "allowedSources": REMOTE_SOURCES, + } + path.write_text(yaml.safe_dump(service, sort_keys=False), encoding="utf-8") + return project + + +def _saddr_rule( + handle: int, source: object | None, family: str = "ip" +) -> dict[str, object]: + """One accepted rule as ``nft -j`` prints ``ip saddr tcp dport 5432``. + + Without a source it is the rule ``tcp dport 5432 accept``. + """ + saddr: list[dict[str, object]] = ( + [] + if source is None + else [ + { + "match": { + "op": "==", + "left": {"payload": {"protocol": family, "field": "saddr"}}, + "right": source, + } + } + ] + ) + return { + "rule": { + "family": "inet", + "table": "cloudfall", + "chain": "input", + "handle": handle, + "comment": "postgresql-main remoteAccess", + "expr": [ + *saddr, + { + "match": { + "op": "==", + "left": {"payload": {"protocol": "tcp", "field": "dport"}}, + "right": 5432, + } + }, + {"accept": None}, + ], + } + } + + +def _rendered_remote_access_rules() -> list[dict[str, object]]: + """Return the rules nftables holds for ``REMOTE_SOURCES``, a /32 bare.""" + return [ + _saddr_rule(10, {"prefix": {"addr": "10.0.0.0", "len": 24}}), + _saddr_rule(11, "10.1.0.7"), + _saddr_rule(12, {"prefix": {"addr": "2001:db8::", "len": 64}}, "ip6"), + ] + + +def _remote_access_observations( + tmp_path: Path, + rules: list[dict[str, object]], + listeners: tuple[str, ...] = (REMOTE_LISTENER,), +) -> Path: + observations = tmp_path / "observed" + shutil.copytree(COMPLIANT, observations) + h1 = observations / "h1.json" + content = json.loads(h1.read_text(encoding="utf-8")) + spec = content["spec"] + table = json.loads(spec["firewall"]["managedTableJson"]) + table["nftables"].extend(rules) + spec["firewall"]["managedTableJson"] = json.dumps(table) + for listener in listeners: + spec["network"]["listeningSockets"] += f"\n{listener}" + h1.write_text(json.dumps(content), encoding="utf-8") + return observations + + +def _h1_check(project: Path, observations: Path, name: str) -> AuditCheck: + inventory = PlatformInventory.from_state(validate_config(project, SCHEMAS)) + report = audit_inventory(inventory, load_observations(observations, SCHEMAS)) + h1_report = next(server for server in report.servers if server.server_id == "h1") + return next(check for check in h1_report.checks if check.check == name) + + +def test_remote_access_rules_and_listener_audit_clean(tmp_path: Path) -> None: + """Regression for #143: the declared remoteAccess was reported as drift.""" + project = _remote_access_project(tmp_path) + observations = _remote_access_observations( + tmp_path, _rendered_remote_access_rules() + ) + inventory = PlatformInventory.from_state(validate_config(project, SCHEMAS)) + + report = audit_inventory(inventory, load_observations(observations, SCHEMAS)) + + assert report.status is AuditStatus.COMPLIANT + allowed = _h1_check(project, observations, "firewall.allowedInbound") + assert allowed.desired == [ + "tcp/22", + "tcp/80", + "tcp/443", + "tcp/5432 from 10.0.0.0/24", + "tcp/5432 from 10.1.0.7/32", + "tcp/5432 from 2001:db8::/64", + ] + assert allowed.observed == allowed.desired + bind = _h1_check(project, observations, "services.bind[postgresql-main]") + assert bind.desired == { + "protocol": "tcp", + "address": "127.0.0.1", + "port": 5432, + "loopbackOnly": False, + "remoteAccess": {"listenAddress": "10.0.0.2"}, + } + assert bind.observed == {"listeners": ["127.0.0.1:5432", "10.0.0.2:5432"]} + + +def test_remote_access_port_open_to_every_address_is_drift(tmp_path: Path) -> None: + project = _remote_access_project(tmp_path) + observations = _remote_access_observations( + tmp_path, [*_rendered_remote_access_rules(), _saddr_rule(13, None)] + ) + + allowed = _h1_check(project, observations, "firewall.allowedInbound") + + assert allowed.status is AuditStatus.DRIFT + assert isinstance(allowed.observed, list) + assert "tcp/5432" in allowed.observed + + +def test_remote_access_from_a_wrong_source_is_drift(tmp_path: Path) -> None: + project = _remote_access_project(tmp_path) + rules = _rendered_remote_access_rules() + rules[0] = _saddr_rule(10, {"prefix": {"addr": "10.0.1.0", "len": 24}}) + observations = _remote_access_observations(tmp_path, rules) + + allowed = _h1_check(project, observations, "firewall.allowedInbound") + + assert allowed.status is AuditStatus.DRIFT + assert isinstance(allowed.observed, list) + assert "tcp/5432 from 10.0.1.0/24" in allowed.observed + assert "tcp/5432 from 10.0.0.0/24" not in allowed.observed + + +@pytest.mark.parametrize( + "listener", + [ + "tcp LISTEN 0 244 10.0.0.3:5432 0.0.0.0:*", + "tcp LISTEN 0 244 0.0.0.0:5432 0.0.0.0:*", + ], +) +def test_remote_access_listener_on_an_undeclared_address_is_drift( + tmp_path: Path, listener: str +) -> None: + project = _remote_access_project(tmp_path) + observations = _remote_access_observations( + tmp_path, _rendered_remote_access_rules(), (REMOTE_LISTENER, listener) + ) + + bind = _h1_check(project, observations, "services.bind[postgresql-main]") + + assert bind.status is AuditStatus.DRIFT + + +def test_remote_access_without_its_listener_is_drift(tmp_path: Path) -> None: + """A PostgreSQL that failed to bind listenAddress has remote access down.""" + project = _remote_access_project(tmp_path) + observations = _remote_access_observations( + tmp_path, _rendered_remote_access_rules(), () + ) + + bind = _h1_check(project, observations, "services.bind[postgresql-main]") + + assert bind.status is AuditStatus.DRIFT + assert bind.observed == {"listeners": ["127.0.0.1:5432"]} + + +def test_a_server_without_remote_access_audits_as_before() -> None: + report = _audit(COMPLIANT) + h1_report = next(server for server in report.servers if server.server_id == "h1") + checks = {check.check: check for check in h1_report.checks} + + assert checks["firewall.allowedInbound"].desired == [ + "tcp/22", + "tcp/80", + "tcp/443", + ] + assert checks["firewall.allowedInbound"].observed == [ + "tcp/22", + "tcp/80", + "tcp/443", + ] + assert checks["services.bind[postgresql-main]"].desired == { + "protocol": "tcp", + "address": "127.0.0.1", + "port": 5432, + "loopbackOnly": True, + } + assert checks["services.bind[postgresql-main]"].observed == { + "listeners": ["127.0.0.1:5432"] + } + + def test_missing_observation_is_unknown( tmp_path: Path, capsys: pytest.CaptureFixture[str],