diff --git a/changelog.d/181-catalog-inputs-as-data.md b/changelog.d/181-catalog-inputs-as-data.md new file mode 100644 index 0000000..db2ab0d --- /dev/null +++ b/changelog.d/181-catalog-inputs-as-data.md @@ -0,0 +1,3 @@ +### Security + +- A catalog operation's inputs are never run as an Ansible template: `cloudfall operations propose`, `cloudfall decisions approve` and `cloudfall agent investigate` hand them to the team's playbook in `--extra-vars` with every string marked unsafe (`{"__ansible_unsafe": ...}`), so an input holding `{{ ... }}` reaches the playbook as written, and integers and booleans keep their types. Before, an input such as `{{ lookup('pipe', '...') }}` ran on the management host with the operator's rights when the playbook started, and an honest input holding `{{` failed with a template syntax error. A team playbook that relied on an input being evaluated as Jinja now gets the literal text. The decision record's `inputs` are unchanged, and recordings made before this change replay as before (#181) diff --git a/sdk/src/cloudfall/decision.py b/sdk/src/cloudfall/decision.py index 707b299..105add4 100644 --- a/sdk/src/cloudfall/decision.py +++ b/sdk/src/cloudfall/decision.py @@ -30,6 +30,7 @@ from jsonschema.exceptions import ValidationError from cloudfall.ansible_api import callback_plugin_path +from cloudfall.ansible_data import as_ansible_data from cloudfall.catalog import RiskLevel, TargetScope from cloudfall.domain import ResourceId @@ -870,9 +871,10 @@ def run_command( if invocation.pattern is not None: argv.extend(("--limit", invocation.pattern)) if invocation.inputs: - argv.extend( - ("--extra-vars", json.dumps(dict(invocation.inputs), sort_keys=True)) - ) + # Ansible templates extra vars: an input is marked as data so its + # text reaches the playbook as written, never run as Jinja (#181). + inputs = as_ansible_data(dict(invocation.inputs)) + argv.extend(("--extra-vars", json.dumps(inputs, sort_keys=True))) argv.append(str(invocation.repository / invocation.playbook)) configuration = invocation.repository / _ANSIBLE_CONFIG_FILE team = configuration if configuration.is_file() else None diff --git a/sdk/src/cloudfall/recording.py b/sdk/src/cloudfall/recording.py index 3efb98e..3c3b650 100644 --- a/sdk/src/cloudfall/recording.py +++ b/sdk/src/cloudfall/recording.py @@ -18,6 +18,7 @@ from pathlib import Path from typing import TYPE_CHECKING, cast +from cloudfall.ansible_data import unmarked from cloudfall.decision import HOST_STATS_VARIABLE if TYPE_CHECKING: @@ -138,7 +139,13 @@ def _invocation( arguments = list(argv) pattern = _value(arguments, "--limit") extra = _value(arguments, "--extra-vars") - inputs = cast("Mapping[str, object]", json.loads(extra)) if extra else {} + # The inputs are marked as Ansible data on the argv; a recording keeps + # them as the decision record does, unmarked, so they are read back so. + inputs = ( + cast("Mapping[str, object]", json.loads(extra, object_hook=unmarked)) + if extra + else {} + ) playbook = arguments[-1] root = f"{repository}/" return playbook.removeprefix(root), pattern, inputs diff --git a/sdk/tests/test_decision.py b/sdk/tests/test_decision.py index 0714439..7d9531c 100644 --- a/sdk/tests/test_decision.py +++ b/sdk/tests/test_decision.py @@ -10,6 +10,7 @@ from typing import TYPE_CHECKING import pytest +from cloudfall.ansible_data import unmarked from cloudfall.app import app from cloudfall.catalog import RiskLevel, TargetScope, load_catalog from cloudfall.cli import main @@ -57,6 +58,33 @@ playbook: playbooks/health.yml """ +ECHO = """\ +id: echo +playbook: playbooks/echo.yml +risk: read +targets: fleet +inputs: + message: string + count: integer + force: boolean +""" + +# Writes the inputs as the play sees them, even in check mode, and their +# types with them: a number or a boolean marked as text would arrive quoted. +ECHO_PLAYBOOK = """\ +--- +- hosts: localhost + connection: local + gather_facts: false + tasks: + - name: Keep the inputs + check_mode: false + ansible.builtin.copy: + content: "{{{{ dict(message=message, count=count, force=force) | to_json }}}}" + dest: {seen} + mode: "0644" +""" + LATER = datetime(2026, 9, 21, 14, 45, 0, tzinfo=UTC) LATEST = datetime(2026, 9, 21, 15, 0, 0, tzinfo=UTC) @@ -225,10 +253,48 @@ def test_the_check_run_is_check_mode_with_a_diff(tmp_path: Path) -> None: assert "--check" in argv assert "--diff" in argv assert argv[argv.index("--limit") + 1] == "web-1" - assert json.loads(argv[argv.index("--extra-vars") + 1]) == {"version": "1.4.0"} + extra_vars = argv[argv.index("--extra-vars") + 1] + assert json.loads(extra_vars, object_hook=unmarked) == {"version": "1.4.0"} assert argv[-1].endswith("playbooks/deploy.yml") +def test_inputs_reach_the_playbook_as_data_never_as_templates( + tmp_path: Path, +) -> None: + """A string input holding Jinja is text to the team's playbook (#181). + + Ansible templates extra vars, so an unmarked ``{{ lookup('pipe', ...) }}`` + would run on the management host when the playbook starts. + """ + marker = tmp_path / "pwned" + payload = "{{ lookup('pipe', 'touch " + str(marker) + "') }}" + seen = tmp_path / "inputs.json" + repository = _repository(tmp_path, echo=ECHO) + (repository / "playbooks" / "echo.yml").write_text( + ECHO_PLAYBOOK.format(seen=seen), encoding="utf-8" + ) + + decision = propose( + _proposal( + repository, + "echo", + target=None, + inputs={"message": payload, "count": 3, "force": True}, + ), + _store(repository), + lambda: MOMENT, + ) + + assert decision.check.exit_code == 0 + assert not marker.exists() + assert json.loads(seen.read_text(encoding="utf-8")) == { + "message": payload, + "count": 3, + "force": True, + } + assert decision.inputs == {"message": payload, "count": 3, "force": True} + + def test_the_team_configuration_runs_their_playbook(tmp_path: Path) -> None: """Their playbook needs their roles and their connection settings.""" repository = _repository(tmp_path) diff --git a/sdk/tests/test_host_stats.py b/sdk/tests/test_host_stats.py index d2e1ad2..f7ee49c 100644 --- a/sdk/tests/test_host_stats.py +++ b/sdk/tests/test_host_stats.py @@ -184,6 +184,30 @@ def test_a_replay_leaves_what_the_callback_would_have_written( assert recorded.check.unchanged == replayed.check.unchanged == ("web-2",) +def test_a_replay_matches_inputs_holding_jinja_as_recorded(tmp_path: Path) -> None: + """The argv marks inputs as data; the recording keeps them as written (#181).""" + repository = _repository(tmp_path) + inputs: dict[str, object] = {"version": "{{ 1.4.0 }}", "force": True} + recorded = propose( + _proposal(repository, inputs=inputs), + _store(repository), + lambda: MOMENT, + _changed_web_1, + ) + replayed_repository = _repository(tmp_path / "replay") + + replayed = propose( + _proposal(replayed_repository, inputs=inputs), + _store(replayed_repository), + lambda: MOMENT, + Recording.load(repository / "decisions").runner(replayed_repository), + ) + + assert recorded.inputs == replayed.inputs == inputs + assert replayed.check.exit_code == 0 + assert replayed.check.changed == ("web-1",) + + def _changed_web_1( _: Sequence[str], environment: Mapping[str, str], diff: Path ) -> int: