From 1bfbfd90f6ca22bddd515e5e551fc2f925458fda Mon Sep 17 00:00:00 2001 From: Miguel Jacq Date: Sun, 28 Jun 2026 20:34:06 +1000 Subject: [PATCH] Escape verbatim JSON and defend against jinja in it. Add option to salt role prefix to guard against malicious injection of var names --- enroll/jinjaturtle.py | 64 +++++++++++++++++++++++++++++++++- tests/test_jinjaturtle.py | 73 ++++++++++++++++++++++++++++++++++----- 2 files changed, 127 insertions(+), 10 deletions(-) diff --git a/enroll/jinjaturtle.py b/enroll/jinjaturtle.py index f3fe8a7..38c6f61 100644 --- a/enroll/jinjaturtle.py +++ b/enroll/jinjaturtle.py @@ -1,7 +1,9 @@ from __future__ import annotations import hashlib +import os import re +import secrets import shutil import subprocess # nosec import tempfile @@ -206,6 +208,61 @@ def jinjify_artifact( ) +def _resolve_var_prefix_salt() -> str: + """Return the salt mixed into generated JinjaTurtle variable prefixes. + + Rationale and the security/reproducibility trade-off + ---------------------------------------------------- + The generated variable namespace is ``_jt__``. The salt + exists as *defence in depth* for one specific attack: a malicious harvested + config (e.g. a JSON file) that injects a key referencing an Enroll-declared + variable, e.g. ``{{ _jt__port }}``. To pre-compute that name at + harvest-authoring time the attacker must know the salt. + + This is deliberately *not* random-per-run by default. JinjaTurtle already + closes the injection directly (it escapes verbatim keys and an independent + output-safety gate rejects any Jinja in a JSON key), so the salt is a belt to + that suspenders. A random-per-run salt would change every generated variable + name on every ``manifest`` run, producing spurious churn in any + version-controlled manifest tree and in tools that diff regenerated output. + Note this never affected ``enroll diff``, which compares *harvest bundles* + (state.json + artifact content hashes) and never reads generated variable + names -- but it would have churned a git-tracked Ansible manifest. + + Default behaviour is therefore a fixed, reproducible namespace: + + * ``ENROLL_JINJATURTLE_VAR_SALT=`` -- operator-pinned salt. Use this to + make the namespace unpredictable to a config author while staying stable and + reproducible across runs/machines (recommended when generating manifests + from untrusted harvests into a tracked repo). ``random`` is special-cased to + mean "fresh per process". + * unset -- a fixed default salt. Output is fully reproducible; security relies + on JinjaTurtle's escaping + key gate, which fully close the issue. + """ + + override = os.environ.get("ENROLL_JINJATURTLE_VAR_SALT") + if override is None or override == "": + # Stable default: reproducible output, no manifest churn. The vulnerability + # is already closed in JinjaTurtle; this fixed namespace is sufficient. + return "en" + if override == "random": + # Opt-in: unpredictable per process (maximally defensive, churns output). + return secrets.token_hex(4) + return re.sub(r"[^A-Za-z0-9]+", "", override)[:16] or "en" + + +# Cache holder; resolved lazily on first use so an operator (or test) can set +# ENROLL_JINJATURTLE_VAR_SALT before the first manifest call and have it honoured. +_VAR_PREFIX_SALT: Optional[str] = None + + +def _var_prefix_salt() -> str: + global _VAR_PREFIX_SALT + if _VAR_PREFIX_SALT is None: + _VAR_PREFIX_SALT = _resolve_var_prefix_salt() + return _VAR_PREFIX_SALT + + def managed_file_var_prefix(role_name: str, src_rel: str) -> str: """Return a JinjaTurtle-safe variable prefix for one managed file. @@ -215,9 +272,14 @@ def managed_file_var_prefix(role_name: str, src_rel: str) -> str: Always include a ``jt`` namespace and the relative artifact path so harvested config keys cannot produce Enroll-owned variables such as ``_managed_files``. + + A salt segment is mixed in (see ``_resolve_var_prefix_salt``). It is a fixed, + reproducible value by default and can be pinned or randomised via + ``ENROLL_JINJATURTLE_VAR_SALT`` as defence in depth against an injected key + that references an Enroll-declared variable. """ - raw = f"{role_name}_jt_{src_rel}" + raw = f"{role_name}_jt_{_var_prefix_salt()}_{src_rel}" safe = re.sub(r"[^A-Za-z0-9_]+", "_", raw).strip("_").lower() safe = re.sub(r"_+", "_", safe) if not safe: diff --git a/tests/test_jinjaturtle.py b/tests/test_jinjaturtle.py index 4ce5d28..9d43650 100644 --- a/tests/test_jinjaturtle.py +++ b/tests/test_jinjaturtle.py @@ -110,11 +110,16 @@ def test_manifest_uses_jinjaturtle_templates_and_does_not_copy_raw( jinjaturtle_mod, "find_jinjaturtle_cmd", lambda: "/usr/bin/jinjaturtle" ) + # Pin the per-run var-prefix salt so generated variable names are + # deterministic for this test. + monkeypatch.setattr(jinjaturtle_mod, "_VAR_PREFIX_SALT", "t") + expected_prefix = jinjaturtle_mod.managed_file_var_prefix("foo", "etc/foo.ini") + # Stub jinjaturtle output. def fake_run_jinjaturtle( jt_exe: str, src_path: str, *, role_name: str, force_format=None ): - assert role_name == "foo_jt_etc_foo_ini" + assert role_name == expected_prefix return JinjifyResult( template_text=f"[main]\nkey = {{{{ {role_name}_key }}}}\n", vars_text=f"{role_name}_key: 1\n", @@ -134,7 +139,7 @@ def test_manifest_uses_jinjaturtle_templates_and_does_not_copy_raw( # Defaults should include jinjaturtle vars. defaults = (role_dir / "defaults" / "main.yml").read_text(encoding="utf-8") - assert "foo_jt_etc_foo_ini_key: 1" in defaults + assert f"{expected_prefix}_key: 1" in defaults def test_openssh_paths_are_jinjaturtle_supported_and_forced_to_ssh() -> None: @@ -171,6 +176,10 @@ def test_jinjify_managed_files_namespaces_multiple_templates( ) monkeypatch.setattr(jinjaturtle_mod, "run_jinjaturtle", fake_run_jinjaturtle) + monkeypatch.setattr(jinjaturtle_mod, "_VAR_PREFIX_SALT", "t") + + pa = jinjaturtle_mod.managed_file_var_prefix("foo", "etc/foo/a.yaml") + pb = jinjaturtle_mod.managed_file_var_prefix("foo", "etc/foo/b.yaml") templated, vars_text = jinjify_managed_files( bundle, @@ -188,14 +197,14 @@ def test_jinjify_managed_files_namespaces_multiple_templates( assert templated == {"etc/foo/a.yaml", "etc/foo/b.yaml"} assert calls == [ - ("a.yaml", "foo_jt_etc_foo_a_yaml"), - ("b.yaml", "foo_jt_etc_foo_b_yaml"), + ("a.yaml", pa), + ("b.yaml", pb), ] - assert "foo_jt_etc_foo_a_yaml_ignore: []" in vars_text - assert "foo_jt_etc_foo_b_yaml_ignore: []" in vars_text + assert f"{pa}_ignore: []" in vars_text + assert f"{pb}_ignore: []" in vars_text assert (template_root / "etc" / "foo" / "a.yaml.j2").read_text( encoding="utf-8" - ) == "ignore: {{ foo_jt_etc_foo_a_yaml_ignore }}\n" + ) == f"ignore: {{{{ {pa}_ignore }}}}\n" def test_jinjify_managed_files_rejects_templates_with_missing_defaults( @@ -254,6 +263,8 @@ def test_jinjify_managed_files_always_namespaces_single_template( ) monkeypatch.setattr(jinjaturtle_mod, "run_jinjaturtle", fake_run_jinjaturtle) + monkeypatch.setattr(jinjaturtle_mod, "_VAR_PREFIX_SALT", "t") + prefix = jinjaturtle_mod.managed_file_var_prefix("foo", "etc/foo.yaml") templated, vars_text = jinjify_managed_files( bundle, @@ -267,8 +278,8 @@ def test_jinjify_managed_files_always_namespaces_single_template( ) assert templated == {"etc/foo.yaml"} - assert calls == ["foo_jt_etc_foo_yaml"] - assert "foo_jt_etc_foo_yaml_managed_files: []" in vars_text + assert calls == [prefix] + assert f"{prefix}_managed_files: []" in vars_text assert "foo_managed_files:" not in vars_text @@ -307,6 +318,50 @@ def test_jinjify_managed_files_rejects_reserved_role_variable_collision( assert not (template_root / "etc" / "foo.yaml.j2").exists() +def test_malicious_json_key_falls_back_to_raw_copy(monkeypatch, tmp_path: Path): + """A harvested JSON config that injects a Jinja construct in an object key + must not produce a template. The real JinjaTurtle output-safety gate refuses + it (exit 2), so Enroll falls back to copying the raw file verbatim. + + Uses the real jinjaturtle binary when present; skipped otherwise. + """ + import shutil as _shutil + + import pytest + + from enroll.jinjaturtle import jinjify_managed_files + + jt_exe = _shutil.which("jinjaturtle") + if not jt_exe: + pytest.skip("jinjaturtle binary not on PATH") + + bundle = tmp_path / "bundle" + template_root = tmp_path / "templates" + artifact = bundle / "artifacts" / "foo" / "etc" / "app.json" + artifact.parent.mkdir(parents=True, exist_ok=True) + # Injected key references a (predictable) Enroll-declared variable name; the + # JSON-key gate must still refuse it regardless of the salt. + artifact.write_text( + '{ "{{ ansible_hostname }}": "x", "port": 8080 }', encoding="utf-8" + ) + + templated, vars_text = jinjify_managed_files( + bundle, + "foo", + template_root, + [{"path": "/etc/app.json", "src_rel": "etc/app.json"}], + jt_exe=jt_exe, + jt_enabled=True, + overwrite_templates=True, + role_name="foo", + ) + + # No template emitted; the raw file is left to be copied verbatim downstream. + assert templated == set() + assert vars_text == "" + assert not (template_root / "etc" / "app.json.j2").exists() + + def test_jinjify_artifact_rejects_unsafe_src_rel(monkeypatch, tmp_path: Path): from enroll.jinjaturtle import jinjify_artifact