From 44d1a22db418e76f1cb93c3a0d278f2eb1a715e7 Mon Sep 17 00:00:00 2001 From: Miguel Jacq Date: Sun, 28 Jun 2026 18:17:45 +1000 Subject: [PATCH] hardlinked source files are refused, remote schema URLs require an explicit flag, generated harvest artifacts use the hardened bundle writer, and task/handler/playbook YAML writes go through one safety gate. --- enroll/accounts.py | 6 +++- enroll/ansible.py | 55 +++++++++++++++++++++--------------- enroll/capture.py | 2 ++ enroll/cli.py | 14 +++++++-- enroll/harvest.py | 12 ++++---- enroll/ignore.py | 6 ++++ enroll/validate.py | 17 ++++++++--- tests/test_harvest_safety.py | 46 ++++++++++++++++++++++++++++++ tests/test_ignore.py | 26 +++++++++++++++++ tests/test_render_safety.py | 13 +++++++++ tests/test_validate.py | 42 +++++++++++++++++++++++++-- 11 files changed, 202 insertions(+), 37 deletions(-) diff --git a/enroll/accounts.py b/enroll/accounts.py index 60a8415..9852853 100644 --- a/enroll/accounts.py +++ b/enroll/accounts.py @@ -186,7 +186,11 @@ def _read_first_existing_text( try: fd = open_no_follow_path(path) st = os.fstat(fd) - if not stat.S_ISREG(st.st_mode) or st.st_size > max_bytes: + if ( + not stat.S_ISREG(st.st_mode) + or st.st_nlink > 1 + or st.st_size > max_bytes + ): continue data = os.read(fd, max_bytes + 1) if len(data) > max_bytes: diff --git a/enroll/ansible.py b/enroll/ansible.py index dacfc13..f50a19c 100644 --- a/enroll/ansible.py +++ b/enroll/ansible.py @@ -526,6 +526,16 @@ def _role_tag(role: str) -> str: return f"role_{safe}" +def _write_generated_task_yaml(path: str, text: str, *, label: str) -> None: + """Write generated task/handler/playbook YAML through one safety gate.""" + + body = text.rstrip() + "\n" + assert_generated_yaml_safe(body, label=label) + os.makedirs(os.path.dirname(path) or ".", exist_ok=True) + with open(path, "w", encoding="utf-8") as f: + f.write(body) + + def _write_playbook_all(path: str, roles: List[str]) -> None: pb_lines = [ "---", @@ -540,9 +550,7 @@ def _write_playbook_all(path: str, roles: List[str]) -> None: pb_lines.append(f" - role: {safe}") pb_lines.append(f" tags: [{_role_tag(safe)}]") text = "\n".join(pb_lines) + "\n" - assert_generated_yaml_safe(text, label="playbook.yml") - with open(path, "w", encoding="utf-8") as f: - f.write(text) + _write_generated_task_yaml(path, text, label="playbook.yml") def _write_playbook_host(path: str, fqdn: str, roles: List[str]) -> None: @@ -560,9 +568,7 @@ def _write_playbook_host(path: str, fqdn: str, roles: List[str]) -> None: pb_lines.append(f" - role: {safe}") pb_lines.append(f" tags: [{_role_tag(safe)}]") text = "\n".join(pb_lines) + "\n" - assert_generated_yaml_safe(text, label="host playbook") - with open(path, "w", encoding="utf-8") as f: - f.write(text) + _write_generated_task_yaml(path, text, label="host playbook") def _ensure_ansible_cfg(cfg_path: str) -> None: @@ -772,16 +778,16 @@ def _write_ansible_role( # and keeps all harvested data in variable files; if any harvested value ever # leaked into this text and changed its shape, fail closed here rather than # emit a poisoned playbook. - assert_generated_yaml_safe(tasks, label=f"role '{role}' tasks/main.yml") - assert_generated_yaml_safe(handlers, label=f"role '{role}' handlers/main.yml") - - with open(os.path.join(role_dir, "tasks", "main.yml"), "w", encoding="utf-8") as f: - f.write(tasks.rstrip() + "\n") - - with open( - os.path.join(role_dir, "handlers", "main.yml"), "w", encoding="utf-8" - ) as f: - f.write(handlers.rstrip() + "\n") + _write_generated_task_yaml( + os.path.join(role_dir, "tasks", "main.yml"), + tasks, + label=f"role '{role}' tasks/main.yml", + ) + _write_generated_task_yaml( + os.path.join(role_dir, "handlers", "main.yml"), + handlers, + label=f"role '{role}' handlers/main.yml", + ) _write_role_meta(role_dir, collections) @@ -1798,13 +1804,16 @@ def _write_managed_files_role( _write_role_defaults(role_dir, vars_map) tasks = _render_role_tasks(AnsibleRole(role), managed_content=True) - with open(os.path.join(role_dir, "tasks", "main.yml"), "w", encoding="utf-8") as f: - f.write(tasks.rstrip() + "\n") - - with open( - os.path.join(role_dir, "handlers", "main.yml"), "w", encoding="utf-8" - ) as f: - f.write(handlers.rstrip() + "\n") + _write_generated_task_yaml( + os.path.join(role_dir, "tasks", "main.yml"), + tasks, + label=f"role '{role}' tasks/main.yml", + ) + _write_generated_task_yaml( + os.path.join(role_dir, "handlers", "main.yml"), + handlers, + label=f"role '{role}' handlers/main.yml", + ) with open(os.path.join(role_dir, "meta", "main.yml"), "w", encoding="utf-8") as f: f.write("---\ndependencies: []\n") diff --git a/enroll/capture.py b/enroll/capture.py index 66065e6..78ecc47 100644 --- a/enroll/capture.py +++ b/enroll/capture.py @@ -100,6 +100,8 @@ def copy_into_bundle( st = os.fstat(fd) if not stat.S_ISREG(st.st_mode): raise OSError("refusing to copy non-regular source") + if st.st_nlink > 1: + raise OSError("refusing to copy hardlinked source") chunks: list[bytes] = [] while True: chunk = os.read(fd, 1024 * 1024) diff --git a/enroll/cli.py b/enroll/cli.py index 934955b..604991e 100644 --- a/enroll/cli.py +++ b/enroll/cli.py @@ -895,8 +895,17 @@ def main() -> None: v.add_argument( "--schema", help=( - "Optional JSON schema source (file path or https:// URL). " - "If omitted, uses the schema vendored in the enroll codebase." + "Optional JSON schema source. File paths are loaded by default; " + "http(s) URLs require --allow-remote-schema. If omitted, uses " + "the schema vendored in the enroll codebase." + ), + ) + v.add_argument( + "--allow-remote-schema", + action="store_true", + help=( + "Allow --schema to fetch an http(s) URL. Disabled by default so " + "validation never makes network requests unless explicitly requested." ), ) v.add_argument( @@ -1066,6 +1075,7 @@ def main() -> None: sops_mode=bool(getattr(args, "sops", False)), schema=getattr(args, "schema", None), no_schema=bool(getattr(args, "no_schema", False)), + allow_remote_schema=bool(getattr(args, "allow_remote_schema", False)), ) fmt = str(getattr(args, "format", "text")) diff --git a/enroll/harvest.py b/enroll/harvest.py index 43eb002..5d8d5b9 100644 --- a/enroll/harvest.py +++ b/enroll/harvest.py @@ -31,7 +31,7 @@ from .harvest_types import ( SysctlSnapshot, ) -from .capture import capture_file +from .capture import capture_file, write_bytes_into_bundle from . import system_paths from .package_hints import package_section_from_installations, safe_name @@ -187,10 +187,12 @@ def _write_generated_artifact( bundle_dir: str, role_name: str, src_rel: str, content: str ) -> None: """Write a generated harvest artifact that did not exist as a file on disk.""" - dst = os.path.join(bundle_dir, "artifacts", role_name, src_rel) - os.makedirs(os.path.dirname(dst), exist_ok=True) - with open(dst, "w", encoding="utf-8") as f: - f.write(content) + + # Keep generated artifacts on the same no-follow/exclusive-create path as + # copied source artifacts. This preserves the invariant that nothing under + # artifacts/ is written via a plain open() that could follow a symlink if a + # directory were unexpectedly reused or raced. + write_bytes_into_bundle(bundle_dir, role_name, src_rel, content.encode("utf-8")) _SYSCTL_KEY_RE = re.compile(r"^[A-Za-z0-9_.-]+$") diff --git a/enroll/ignore.py b/enroll/ignore.py index b2bebaf..d93ba74 100644 --- a/enroll/ignore.py +++ b/enroll/ignore.py @@ -355,6 +355,12 @@ class IgnorePolicy: if not stat.S_ISREG(st.st_mode): return "not_regular_file", None + if st.st_nlink > 1: + # A hardlink gives a file another safe-looking pathname. When + # Enroll is run as root, path-based deny rules such as + # /etc/shadow must not be bypassed by copying the same inode via + # an attacker-controlled alias under an otherwise allowed tree. + return "hardlink_source", None if st.st_size > self.max_file_bytes: return "too_large", None diff --git a/enroll/validate.py b/enroll/validate.py index e8c65ea..f500b55 100644 --- a/enroll/validate.py +++ b/enroll/validate.py @@ -58,12 +58,15 @@ def _default_schema_path() -> Path: return Path(__file__).resolve().parent / "schema" / "state.schema.json" -def _load_schema(schema: Optional[str]) -> Dict[str, Any]: +def _load_schema( + schema: Optional[str], *, allow_remote_schema: bool = False +) -> Dict[str, Any]: """Load a JSON schema. If schema is None, load the vendored schema. - If schema begins with http(s)://, fetch it. - Otherwise, treat it as a local file path. + If schema begins with http(s)://, fetch it only when remote schema + loading was explicitly enabled by the caller. Otherwise, treat it as a + local file path. """ if not schema: @@ -72,6 +75,11 @@ def _load_schema(schema: Optional[str]) -> Dict[str, Any]: return json.load(f) if schema.startswith("http://") or schema.startswith("https://"): + if not allow_remote_schema: + raise ValueError( + "remote schema URLs are disabled by default; use " + "--allow-remote-schema only when the schema source is trusted" + ) with urllib.request.urlopen(schema, timeout=10) as resp: # nosec data = resp.read() return json.loads(data.decode("utf-8")) @@ -136,6 +144,7 @@ def validate_harvest( sops_mode: bool = False, schema: Optional[str] = None, no_schema: bool = False, + allow_remote_schema: bool = False, ) -> ValidationResult: """Validate an enroll harvest bundle. @@ -165,7 +174,7 @@ def validate_harvest( if not no_schema: try: - sch = _load_schema(schema) + sch = _load_schema(schema, allow_remote_schema=allow_remote_schema) validator = jsonschema.Draft202012Validator(sch) for err in sorted(validator.iter_errors(state), key=str): ptr = _json_pointer(err) diff --git a/tests/test_harvest_safety.py b/tests/test_harvest_safety.py index ef01cc5..a0002fd 100644 --- a/tests/test_harvest_safety.py +++ b/tests/test_harvest_safety.py @@ -320,3 +320,49 @@ def test_ensure_private_empty_dir_creates_private_dir(tmp_path: Path): out = hs.ensure_private_empty_dir(tmp_path / "new-cache", label="cache") assert out.is_dir() assert (out.stat().st_mode & 0o777) == 0o700 + + +def test_capture_file_excludes_hardlinked_source(tmp_path: Path): + source = tmp_path / "source.conf" + source.write_text("safe=true\n", encoding="utf-8") + alias = tmp_path / "alias.conf" + os.link(source, alias) + bundle = tmp_path / "bundle" + bundle.mkdir() + + managed: list[ManagedFile] = [] + excluded: list[ExcludedFile] = [] + ok = capture_file( + bundle_dir=str(bundle), + role_name="role", + abs_path=str(alias), + reason="test", + policy=IgnorePolicy(), + path_filter=PathFilter(), + managed_out=managed, + excluded_out=excluded, + ) + + assert ok is False + assert managed == [] + assert excluded == [ExcludedFile(path=str(alias), reason="hardlink_source")] + assert not (bundle / "artifacts" / "role" / str(alias).lstrip("/")).exists() + + +def test_generated_artifact_writer_refuses_existing_symlink(tmp_path: Path): + from enroll.harvest import _write_generated_artifact + + bundle = tmp_path / "bundle" + dst_dir = bundle / "artifacts" / "firewall_runtime" / "runtime" + dst_dir.mkdir(parents=True) + target = tmp_path / "target" + target.write_text("old\n", encoding="utf-8") + link = dst_dir / "iptables-v4.save" + os.symlink(target, link) + + with pytest.raises(OSError): + _write_generated_artifact( + str(bundle), "firewall_runtime", "runtime/iptables-v4.save", "new\n" + ) + + assert target.read_text(encoding="utf-8") == "old\n" diff --git a/tests/test_ignore.py b/tests/test_ignore.py index 2f138ef..87f0757 100644 --- a/tests/test_ignore.py +++ b/tests/test_ignore.py @@ -416,3 +416,29 @@ def test_inspect_file_still_captures_normal_nested_file(tmp_path: Path): assert reason is None assert inspection is not None assert inspection.data == b"workers=4\n" + + +def test_inspect_file_refuses_hardlinked_source(tmp_path: Path): + pol = IgnorePolicy() + original = tmp_path / "original.conf" + original.write_text("safe=true\n", encoding="utf-8") + alias = tmp_path / "alias.conf" + os.link(original, alias) + + reason, inspection = pol.inspect_file(str(alias)) + + assert reason == "hardlink_source" + assert inspection is None + + +def test_inspect_file_refuses_hardlinked_source_even_in_dangerous_mode(tmp_path: Path): + pol = IgnorePolicy(dangerous=True) + original = tmp_path / "original.conf" + original.write_text("safe=true\n", encoding="utf-8") + alias = tmp_path / "alias.conf" + os.link(original, alias) + + reason, inspection = pol.inspect_file(str(alias)) + + assert reason == "hardlink_source" + assert inspection is None diff --git a/tests/test_render_safety.py b/tests/test_render_safety.py index 4efebd7..6f38215 100644 --- a/tests/test_render_safety.py +++ b/tests/test_render_safety.py @@ -106,3 +106,16 @@ def test_ansible_unsafe_data_still_tags_jinja_values(): assert not isinstance(out["safe"], AnsibleUnsafeText) assert is_ansible_template_like("{{ x }}") assert not is_ansible_template_like("plain") + + +def test_write_generated_task_yaml_uses_safety_gate(tmp_path): + from enroll.ansible import _write_generated_task_yaml + + path = tmp_path / "tasks.yml" + + with pytest.raises(RenderSafetyError): + _write_generated_task_yaml( + str(path), "---\nkey: value\n", label="test tasks/main.yml" + ) + + assert not path.exists() diff --git a/tests/test_validate.py b/tests/test_validate.py index 002d2cf..83f6764 100644 --- a/tests/test_validate.py +++ b/tests/test_validate.py @@ -305,16 +305,54 @@ def test_validate_harvest_invalid_json(tmp_path: Path): assert any("failed to parse" in e for e in result.errors) -def test_validate_harvest_schema_error(tmp_path: Path): +def test_validate_harvest_rejects_remote_schema_by_default(tmp_path: Path): bundle_dir = tmp_path / "bundle" bundle_dir.mkdir() state_file = bundle_dir / "state.json" state_file.write_text("{}", encoding="utf-8") + result = validate_harvest( str(bundle_dir), schema="https://invalid.invalid/schema.json" ) + assert result.ok is False - assert any("failed to load/validate schema" in e for e in result.errors) + assert any("remote schema URLs are disabled" in e for e in result.errors) + + +def test_validate_harvest_allows_remote_schema_when_explicit( + monkeypatch: pytest.MonkeyPatch, tmp_path: Path +): + bundle_dir = tmp_path / "bundle" + bundle_dir.mkdir() + state_file = bundle_dir / "state.json" + state_file.write_text("{}", encoding="utf-8") + + class _Response: + def __enter__(self): + return self + + def __exit__(self, exc_type, exc, tb): + return False + + def read(self): + return b'{"type":"object"}' + + called = [] + + def _fake_urlopen(url: str, timeout: int): + called.append((url, timeout)) + return _Response() + + monkeypatch.setattr("enroll.validate.urllib.request.urlopen", _fake_urlopen) + + result = validate_harvest( + str(bundle_dir), + schema="https://example.invalid/schema.json", + allow_remote_schema=True, + ) + + assert called == [("https://example.invalid/schema.json", 10)] + assert not any("remote schema URLs are disabled" in e for e in result.errors) def test_validate_harvest_missing_artifact(tmp_path: Path):